refactor: clean up `LedgerEntry.cpp` #5199

mvadari · 2024-11-22T16:41:21Z

High Level Overview of Change

This PR heavily restructures LedgerEntry.cpp to make it easier to read and understand.

Instead of having a nested series of if-elses, which is very difficult to parse, each object now has 3 elements in an array: the parameter name, a parsing function, and the expected ledger entry type.

{jss::account_root, parseAccountRoot, ltACCOUNT_ROOT},

This allows the parsing logic to be greatly simplified by returning early in each of the parsing functions, instead of needing to fall through with a lot of else statements - and this also avoids a lot of extra indenting (which in turn makes the code easier to read). It also makes it easier to audit what ledger entries are missing (e.g. NFT Offers are missing right now), and avoids bugs from forgetting to specify the expected ledger entry type.

It does not change any of the individual ledger entry parsing code, to make this PR easier to follow and review.

Context of Change

LedgerEntry.cpp was a very messy file. I caught a bug in a PR that would have been avoided by this sort of structure.

Type of Change

Refactor (non-breaking change that only restructures code)

API Impact

No change in functionality.

Test Plan

Tests remained mostly unchanged. We may want some basic performance regression tests, but the performance effect should be pretty minimal.

Future Tasks

Add parsing helper functions so that each new ledger entry parser doesn't need to reinvent the wheel.

mvadari · 2024-11-22T16:43:39Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

        {
-            if (context.params.isMember("params") &&


This was only used for the CLI, so the CLI logic was moved to RPCCall instead, where it's cleaner and easier to read.

mvadari · 2024-11-22T16:44:42Z

src/test/rpc/LedgerRPC_test.cpp

-            checkErrorValue(jrr, "malformedRequest", "");
-        }
+        // check both aliases
+        for (auto const& fieldName : {jss::ripple_state, jss::state})


These tests were only updated to cover the two aliases, and are otherwise completely unchanged.

thejohnfreeman · 2024-11-22T17:09:42Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

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


I don't think you need runtime polymorphism here.

Suggested change

using FunctionType =

std::function<std::optional<uint256>(Json::Value const&, Json::Value&)>;

using FunctionType =

std::optional<uint256>(*)(Json::Value const&, Json::Value&);

I'm unfamiliar with the pros and cons of std::function vs C-style pointers, I just assumed the norm was the more modern option.

godexsoft

Looks like a huge improvement @mvadari , well done! 🥇
I left a few nits. In general any of them could be left for another PR - as you wish.
Other than the ones I mentioned, i'd like to see const applied everywhere it can be. I'd also prefer to see some more spacing lines between if statements and stuff like that. Big if conditions could be broken down to helper functions/lambdas and/or constants. Some of them are pretty hard to read as they are now but I understand that's old code, not new one.

godexsoft · 2024-11-22T17:12:49Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

+        return uNodeIndex;
+    }
+    // clang-format off
+    if (


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

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.

godexsoft · 2024-11-22T17:27:03Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

+    // 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>(


Could be const?

Fixed in 3d5ef0f

godexsoft · 2024-11-22T17:27:52Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

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


Can be const

Fixed in 3d5ef0f

godexsoft · 2024-11-22T17:31:35Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

+    if (!lpLedger)
+        return jvResult;
+
+    static LedgerEntry ledgerEntryParsers[] = {


nit: Consider using std::array over C-arrays

Fixed in 3d5ef0f - not sure I like it, though

godexsoft · 2024-11-22T17:37:43Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

@@ -837,43 +889,51 @@ doLedgerEntry(RPC::JsonContext& context)
            // For apiVersion 2 onwards, any parsing failures that throw
            // this


nit: Looks like a formatting issue, maybe due to clang-format and column width limit changes in the past

Fixed in 3d5ef0f

mvadari · 2024-11-22T19:14:20Z

Other than the ones I mentioned, i'd like to see const applied everywhere it can be. I'd also prefer to see some more spacing lines between if statements and stuff like that. Big if conditions could be broken down to helper functions/lambdas and/or constants. Some of them are pretty hard to read as they are now but I understand that's old code, not new one.

Thanks for the review! I added more spacing and I think I added const everywhere where applicable.

Definitely plan on adding more helper functions (every individual parser was written sort of individually, and had to reinvent a lot of wheels, and I think helper functions would do a lot to unify a lot of that). But wanted to save that for a separate PR to make this PR easier to review (since that code is already being moved around pretty significantly).

godexsoft · 2024-11-22T19:55:00Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

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


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{...}).

Fixed in c255d46

codecov · 2024-11-22T21:03:37Z

Codecov Report

Attention: Patch coverage is 79.06977% with 90 lines in your changes missing coverage. Please review.

Project coverage is 77.8%. Comparing base (f64cf91) to head (9d67330).

Files with missing lines	Patch %	Lines
src/xrpld/rpc/handlers/LedgerEntry.cpp	78.9%	89 Missing ⚠️
src/xrpld/net/detail/RPCCall.cpp	87.5%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5199     +/-   ##
=========================================
- Coverage     77.9%   77.8%   -0.1%     
=========================================
  Files          784     784             
  Lines        66681   66676      -5     
  Branches      8162    8137     -25     
=========================================
- Hits         51950   51899     -51     
- Misses       14731   14777     +46

Files with missing lines	Coverage Δ
src/xrpld/rpc/detail/RPCHelpers.cpp	`82.8% <ø> (ø)`
src/xrpld/net/detail/RPCCall.cpp	`94.2% <87.5%> (-0.1%)`	⬇️
src/xrpld/rpc/handlers/LedgerEntry.cpp	`76.0% <78.9%> (-3.8%)`	⬇️

... and 9 files with indirect coverage changes

godexsoft · 2024-11-23T02:55:09Z

src/xrpld/rpc/handlers/LedgerEntry.cpp

-    static std::array<LedgerEntry, 21> ledgerEntryParsers = {
-        LedgerEntry{jss::index, parseIndex, ltANY},
-        LedgerEntry{jss::account_root, parseAccountRoot, ltACCOUNT_ROOT},
+    static auto ledgerEntryParsers = std::to_array<LedgerEntry>({


Awesome 👍

refactor LedgerEntry

f1ec955

mvadari commented Nov 22, 2024

View reviewed changes

thejohnfreeman reviewed Nov 22, 2024

View reviewed changes

godexsoft approved these changes Nov 22, 2024

View reviewed changes

respond to comments

3d5ef0f

mvadari force-pushed the refactor-ledger-entry branch from 6053af1 to 3d5ef0f Compare November 22, 2024 19:06

add more spacing

04fd6e3

godexsoft reviewed Nov 22, 2024

View reviewed changes

more cleanup

c255d46

use std::to_array

ee270ab

godexsoft reviewed Nov 23, 2024

View reviewed changes

Merge branch 'develop' into refactor-ledger-entry

9d67330

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: clean up `LedgerEntry.cpp` #5199

refactor: clean up `LedgerEntry.cpp` #5199

mvadari commented Nov 22, 2024 •

edited

Loading

mvadari Nov 22, 2024

mvadari Nov 22, 2024

thejohnfreeman Nov 22, 2024

mvadari Nov 22, 2024

godexsoft left a comment

godexsoft Nov 22, 2024

mvadari Nov 22, 2024

godexsoft Nov 22, 2024

mvadari Nov 22, 2024

godexsoft Nov 22, 2024

mvadari Nov 22, 2024

godexsoft Nov 22, 2024

mvadari Nov 22, 2024

godexsoft Nov 22, 2024

mvadari Nov 22, 2024

mvadari commented Nov 22, 2024

godexsoft Nov 22, 2024

mvadari Nov 22, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

godexsoft Nov 23, 2024

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

		@@ -837,43 +889,51 @@ doLedgerEntry(RPC::JsonContext& context)
		// For apiVersion 2 onwards, any parsing failures that throw
		// this

refactor: clean up LedgerEntry.cpp #5199

Are you sure you want to change the base?

refactor: clean up LedgerEntry.cpp #5199

Conversation

mvadari commented Nov 22, 2024 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

API Impact

Test Plan

Future Tasks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godexsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvadari commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

refactor: clean up `LedgerEntry.cpp` #5199

refactor: clean up `LedgerEntry.cpp` #5199

mvadari commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading