XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.53k stars 1.47k forks source link

refactor: clean up `LedgerEntry.cpp` #5199

Open mvadari opened 11 hours ago

mvadari commented 11 hours ago

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

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

mvadari commented 9 hours ago

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

codecov[bot] commented 7 hours ago

Codecov Report

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

Project coverage is 77.9%. Comparing base (0ec17b6) to head (ee270ab).

Files with missing lines Patch % Lines
src/xrpld/rpc/handlers/LedgerEntry.cpp 78.9% 89 Missing :warning:
src/xrpld/net/detail/RPCCall.cpp 87.5% 1 Missing :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/5199/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/5199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) ```diff @@ Coverage Diff @@ ## develop #5199 +/- ## ========================================= - Coverage 77.9% 77.9% -0.0% ========================================= Files 782 782 Lines 66622 66619 -3 Branches 8136 8119 -17 ========================================= - Hits 51902 51873 -29 - Misses 14720 14746 +26 ``` | [Files with missing lines](https://app.codecov.io/gh/XRPLF/rippled/pull/5199?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) | Coverage Δ | | |---|---|---| | [src/xrpld/rpc/detail/RPCHelpers.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5199?src=pr&el=tree&filepath=src%2Fxrpld%2Frpc%2Fdetail%2FRPCHelpers.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3hycGxkL3JwYy9kZXRhaWwvUlBDSGVscGVycy5jcHA=) | `82.8% <ø> (ø)` | | | [src/xrpld/net/detail/RPCCall.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5199?src=pr&el=tree&filepath=src%2Fxrpld%2Fnet%2Fdetail%2FRPCCall.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3hycGxkL25ldC9kZXRhaWwvUlBDQ2FsbC5jcHA=) | `94.2% <87.5%> (-0.1%)` | :arrow_down: | | [src/xrpld/rpc/handlers/LedgerEntry.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/5199?src=pr&el=tree&filepath=src%2Fxrpld%2Frpc%2Fhandlers%2FLedgerEntry.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3hycGxkL3JwYy9oYW5kbGVycy9MZWRnZXJFbnRyeS5jcHA=) | `76.0% <78.9%> (-3.8%)` | :arrow_down: | ... and [3 files with indirect coverage changes](https://app.codecov.io/gh/XRPLF/rippled/pull/5199/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/5199/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/5199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)

🚨 Try these New Features: