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.

Sign up for GitHub

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

refactor: clean up LedgerEntry.cpp #5199

Merged
merged 8 commits into from
Dec 4, 2024
Merged

refactor: clean up LedgerEntry.cpp #5199

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f1ec955
refactor LedgerEntry
mvadari Nov 20, 2024
3d5ef0f
respond to comments
mvadari Nov 22, 2024
04fd6e3
add more spacing
mvadari Nov 22, 2024
c255d46
more cleanup
mvadari Nov 22, 2024
ee270ab
use std::to_array
mvadari Nov 22, 2024
9d67330
Merge branch 'develop' into refactor-ledger-entry
mvadari Nov 27, 2024
c955691
Merge branch 'develop' into refactor-ledger-entry
mvadari Dec 4, 2024
81ba7e6
update API Changelog, add `break`
mvadari Dec 4, 2024
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
152 changes: 86 additions & 66 deletions src/xrpld/rpc/handlers/LedgerEntry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ parseIndex(Json::Value const& params, Json::Value& jvResult)
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}

return uNodeIndex;
}

Expand All @@ -88,6 +89,7 @@ parseAccountRoot(Json::Value const& params, Json::Value& jvResult)
jvResult[jss::error] = "malformedAddress";
return std::nullopt;
}

return keylet::account(*account).key;
}

Expand All @@ -100,6 +102,7 @@ parseCheck(Json::Value const& params, Json::Value& jvResult)
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}

return uNodeIndex;
}

Expand All @@ -116,6 +119,7 @@ parseDepositPreauth(Json::Value const& dp, Json::Value& jvResult)
}
return uNodeIndex;
}

// clang-format off
if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be improved by splitting to some constants and/or helper function(s) 👍 Can be done in a later PR though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any cleanup in the individual parsers (more than just adding consts) I would prefer to save for a future PR, to make this one easier to review.

(!dp.isMember(jss::owner) || !dp[jss::owner].isString()) ||
Expand All @@ -135,6 +139,7 @@ parseDepositPreauth(Json::Value const& dp, Json::Value& jvResult)
jvResult[jss::error] = "malformedOwner";
return std::nullopt;
}

if (dp.isMember(jss::authorized))
{
auto const authorized =
Expand All @@ -155,7 +160,8 @@ parseDepositPreauth(Json::Value const& dp, Json::Value& jvResult)
jvResult[jss::error] = "malformedAuthorizedCredentials";
return std::nullopt;
}
auto sorted = credentials::makeSorted(arr);

auto const& sorted = credentials::makeSorted(arr);
if (sorted.empty())
{
jvResult[jss::error] = "malformedAuthorizedCredentials";
Expand Down Expand Up @@ -204,6 +210,7 @@ parseDirectory(Json::Value const& params, Json::Value& jvResult)
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}

if (!uDirRoot.parseHex(params[jss::dir_root].asString()))
{
jvResult[jss::error] = "malformedRequest";
Expand All @@ -222,6 +229,7 @@ parseDirectory(Json::Value const& params, Json::Value& jvResult)
jvResult[jss::error] = "malformedAddress";
return std::nullopt;
}

return keylet::page(keylet::ownerDir(*ownerID), uSubIndex).key;
}

Expand All @@ -237,10 +245,10 @@ parseEscrow(Json::Value const& params, Json::Value& jvResult)
uint256 uNodeIndex;
if (!uNodeIndex.parseHex(params.asString()))
{
uNodeIndex = beast::zero;
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}

return uNodeIndex;
}

Expand Down Expand Up @@ -275,6 +283,7 @@ parseOffer(Json::Value const& params, Json::Value& jvResult)
}
return uNodeIndex;
}

if (!params.isMember(jss::account) || !params.isMember(jss::seq) ||
!params[jss::seq].isIntegral())
{
Expand All @@ -301,6 +310,7 @@ parsePaymentChannel(Json::Value const& params, Json::Value& jvResult)
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}

return uNodeIndex;
}

Expand Down Expand Up @@ -439,6 +449,7 @@ parseBridge(Json::Value const& params, Json::Value& jvResult)
{
return std::nullopt;
}

auto const account =
parseBase58<AccountID>(jsBridgeAccount.asString());
if (!account || account->isZero())
Expand All @@ -452,6 +463,7 @@ parseBridge(Json::Value const& params, Json::Value& jvResult)
STXChainBridge const bridge(params[jss::bridge]);
STXChainBridge::ChainType const chainType =
STXChainBridge::srcChain(account == bridge.lockingChainDoor());

if (account != bridge.door(chainType))
return std::nullopt;

Expand Down Expand Up @@ -504,12 +516,13 @@ parseXChainOwnedClaimID(Json::Value const& claim_id, Json::Value& jvResult)
// four strings defining the bridge (locking_chain_door,
// locking_chain_issue, issuing_chain_door, issuing_chain_issue)
// and the claim id sequence number.
auto lockingChainDoor = parseBase58<AccountID>(
auto const lockingChainDoor = parseBase58<AccountID>(
claim_id[sfLockingChainDoor.getJsonName()].asString());
auto issuingChainDoor = parseBase58<AccountID>(
auto const issuingChainDoor = parseBase58<AccountID>(
claim_id[sfIssuingChainDoor.getJsonName()].asString());
Issue lockingChainIssue, issuingChainIssue;
bool valid = lockingChainDoor && issuingChainDoor;

if (valid)
{
try
Expand All @@ -528,7 +541,7 @@ parseXChainOwnedClaimID(Json::Value const& claim_id, Json::Value& jvResult)

if (valid && claim_id[jss::xchain_owned_claim_id].isIntegral())
{
auto seq = claim_id[jss::xchain_owned_claim_id].asUInt();
auto const seq = claim_id[jss::xchain_owned_claim_id].asUInt();

STXChainBridge bridge_spec(
*lockingChainDoor,
Expand Down Expand Up @@ -579,9 +592,9 @@ parseXChainOwnedCreateAccountClaimID(
// (locking_chain_door, locking_chain_issue, issuing_chain_door,
// issuing_chain_issue) and the create account claim id sequence
// number.
auto lockingChainDoor = parseBase58<AccountID>(
auto const lockingChainDoor = parseBase58<AccountID>(
claim_id[sfLockingChainDoor.getJsonName()].asString());
auto issuingChainDoor = parseBase58<AccountID>(
auto const issuingChainDoor = parseBase58<AccountID>(
claim_id[sfIssuingChainDoor.getJsonName()].asString());
Issue lockingChainIssue, issuingChainIssue;
bool valid = lockingChainDoor && issuingChainDoor;
Expand All @@ -604,7 +617,8 @@ parseXChainOwnedCreateAccountClaimID(
if (valid &&
claim_id[jss::xchain_owned_create_account_claim_id].isIntegral())
{
auto seq = claim_id[jss::xchain_owned_create_account_claim_id].asUInt();
auto const seq =
claim_id[jss::xchain_owned_create_account_claim_id].asUInt();

STXChainBridge bridge_spec(
*lockingChainDoor,
Expand Down Expand Up @@ -644,45 +658,45 @@ parseOracle(Json::Value const& params, Json::Value& jvResult)
}
return uNodeIndex;
}
else if (
!params.isMember(jss::oracle_document_id) ||

if (!params.isMember(jss::oracle_document_id) ||
!params.isMember(jss::account))
{
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}
else
{
auto const& oracle = params;
auto const documentID = [&]() -> std::optional<std::uint32_t> {
auto const& id = oracle[jss::oracle_document_id];
if (id.isUInt() || (id.isInt() && id.asInt() >= 0))
return std::make_optional(id.asUInt());
else if (id.isString())
{
std::uint32_t v;
if (beast::lexicalCastChecked(v, id.asString()))
return std::make_optional(v);
}
return std::nullopt;
}();

auto const account =
parseBase58<AccountID>(oracle[jss::account].asString());
if (!account || account->isZero())
{
jvResult[jss::error] = "malformedAddress";
return std::nullopt;
}
auto const& oracle = params;
auto const documentID = [&]() -> std::optional<std::uint32_t> {
auto const id = oracle[jss::oracle_document_id];
if (id.isUInt() || (id.isInt() && id.asInt() >= 0))
return std::make_optional(id.asUInt());

if (!documentID)
if (id.isString())
{
jvResult[jss::error] = "malformedDocumentID";
return std::nullopt;
std::uint32_t v;
if (beast::lexicalCastChecked(v, id.asString()))
return std::make_optional(v);
}

return keylet::oracle(*account, *documentID).key;
return std::nullopt;
}();

auto const account =
parseBase58<AccountID>(oracle[jss::account].asString());
if (!account || account->isZero())
{
jvResult[jss::error] = "malformedAddress";
return std::nullopt;
}

if (!documentID)
{
jvResult[jss::error] = "malformedDocumentID";
return std::nullopt;
}

return keylet::oracle(*account, *documentID).key;
}

std::optional<uint256>
Expand All @@ -707,9 +721,11 @@ parseCredential(Json::Value const& cred, Json::Value& jvResult)
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}

auto const subject = parseBase58<AccountID>(cred[jss::subject].asString());
auto const issuer = parseBase58<AccountID>(cred[jss::issuer].asString());
auto const credType = strUnHex(cred[jss::credential_type].asString());

if (!subject || subject->isZero() || !issuer || issuer->isZero() ||
!credType || credType->empty())
{
Expand Down Expand Up @@ -763,6 +779,7 @@ parseMPToken(Json::Value const& mptJson, Json::Value& jvResult)
jvResult[jss::error] = "malformedRequest";
return std::nullopt;
}

try
{
auto const mptIssuanceIdStr = mptJson[jss::mpt_issuance_id].asString();
Expand Down Expand Up @@ -790,7 +807,7 @@ parseMPToken(Json::Value const& mptJson, Json::Value& jvResult)
}

using FunctionType =
std::function<std::optional<uint256>(Json::Value const&, Json::Value&)>;
std::optional<uint256> (*)(Json::Value const&, Json::Value&);

struct LedgerEntry
{
Expand All @@ -813,37 +830,41 @@ doLedgerEntry(RPC::JsonContext& context)
if (!lpLedger)
return jvResult;

static LedgerEntry ledgerEntryParsers[] = {
{jss::index, parseIndex, ltANY},
{jss::account_root, parseAccountRoot, ltACCOUNT_ROOT},
static std::array<LedgerEntry, 21> ledgerEntryParsers = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can totally avoid specifying the size here like so: https://godbolt.org/z/6jETseadE
The downside is that you need to wrap each entry with its type (LedgerEntry{...}).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in c255d46

LedgerEntry{jss::index, parseIndex, ltANY},
LedgerEntry{jss::account_root, parseAccountRoot, ltACCOUNT_ROOT},
// TODO: add amendments
{jss::amm, parseAMM, ltAMM},
{jss::bridge, parseBridge, ltBRIDGE},
{jss::check, parseCheck, ltCHECK},
{jss::credential, parseCredential, ltCREDENTIAL},
{jss::deposit_preauth, parseDepositPreauth, ltDEPOSIT_PREAUTH},
{jss::did, parseDID, ltDID},
{jss::directory, parseDirectory, ltDIR_NODE},
{jss::escrow, parseEscrow, ltESCROW},
LedgerEntry{jss::amm, parseAMM, ltAMM},
LedgerEntry{jss::bridge, parseBridge, ltBRIDGE},
LedgerEntry{jss::check, parseCheck, ltCHECK},
LedgerEntry{jss::credential, parseCredential, ltCREDENTIAL},
LedgerEntry{
jss::deposit_preauth, parseDepositPreauth, ltDEPOSIT_PREAUTH},
LedgerEntry{jss::did, parseDID, ltDID},
LedgerEntry{jss::directory, parseDirectory, ltDIR_NODE},
LedgerEntry{jss::escrow, parseEscrow, ltESCROW},
// TODO: add fee, hashes
{jss::mpt_issuance, parseMPTokenIssuance, ltMPTOKEN_ISSUANCE},
{jss::mptoken, parseMPToken, ltMPTOKEN},
LedgerEntry{
jss::mpt_issuance, parseMPTokenIssuance, ltMPTOKEN_ISSUANCE},
LedgerEntry{jss::mptoken, parseMPToken, ltMPTOKEN},
// TODO: add NFT Offers
{jss::nft_page, parseNFTokenPage, ltNFTOKEN_PAGE},
LedgerEntry{jss::nft_page, parseNFTokenPage, ltNFTOKEN_PAGE},
// TODO: add NegativeUNL
{jss::offer, parseOffer, ltOFFER},
{jss::oracle, parseOracle, ltORACLE},
{jss::payment_channel, parsePaymentChannel, ltPAYCHAN},
{jss::ripple_state, parseRippleState, ltRIPPLE_STATE},
LedgerEntry{jss::offer, parseOffer, ltOFFER},
LedgerEntry{jss::oracle, parseOracle, ltORACLE},
LedgerEntry{jss::payment_channel, parsePaymentChannel, ltPAYCHAN},
LedgerEntry{jss::ripple_state, parseRippleState, ltRIPPLE_STATE},
// This is an alias, since the `ledger_data` filter uses jss::state
{jss::state, parseRippleState, ltRIPPLE_STATE},
{jss::ticket, parseTicket, ltTICKET},
{jss::xchain_owned_claim_id,
parseXChainOwnedClaimID,
ltXCHAIN_OWNED_CLAIM_ID},
{jss::xchain_owned_create_account_claim_id,
parseXChainOwnedCreateAccountClaimID,
ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID},
LedgerEntry{jss::state, parseRippleState, ltRIPPLE_STATE},
LedgerEntry{jss::ticket, parseTicket, ltTICKET},
LedgerEntry{
jss::xchain_owned_claim_id,
parseXChainOwnedClaimID,
ltXCHAIN_OWNED_CLAIM_ID},
LedgerEntry{
jss::xchain_owned_create_account_claim_id,
parseXChainOwnedCreateAccountClaimID,
ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID},
};

uint256 uNodeIndex;
Expand Down Expand Up @@ -886,8 +907,7 @@ doLedgerEntry(RPC::JsonContext& context)
{
if (context.apiVersion > 1u)
{
// For apiVersion 2 onwards, any parsing failures that throw
// this
// For apiVersion 2 onwards, any parsing failures that throw this
// exception return an invalidParam error.
jvResult[jss::error] = "invalidParams";
return jvResult;
Expand Down Expand Up @@ -964,7 +984,7 @@ doLedgerEntryGrpc(
return {response, errorStatus};
}

auto key = uint256::fromVoidChecked(request.key());
auto const key = uint256::fromVoidChecked(request.key());
if (!key)
{
grpc::Status errorStatus{
Expand Down
Loading