major refactor of ledger_entry source code and tests#5237
major refactor of ledger_entry source code and tests#5237ximinez merged 21 commits intoXRPLF:developfrom
ledger_entry source code and tests#5237Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5237 +/- ##
=======================================
Coverage 78.6% 78.7%
=======================================
Files 815 816 +1
Lines 71778 71748 -30
Branches 8465 8473 +8
=======================================
- Hits 56453 56447 -6
+ Misses 15325 15301 -24
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
6008f56 to
ee2b76f
Compare
src/libxrpl/json/json_value.cpp
Outdated
| (other == uintValue && value_.real_ >= 0 && | ||
| value_.real_ <= maxUInt) || | ||
| value_.real_ <= maxUInt && | ||
| static_cast<int>(value_.real_) == value_.real_) || |
There was a problem hiding this comment.
You should avoid using == with float point. Consider something like std::fabs(round(value_.real_) - value_.real_) < 1e-10
There was a problem hiding this comment.
We should use std::numeric_limits<double>::epsilon() instead of 1e-10
There was a problem hiding this comment.
Should we cast it to Json::UInt?
src/xrpld/rpc/detail/RPCHelpers.cpp
Outdated
| } | ||
|
|
||
| auto const index = indexValue.asString(); | ||
| if (indexValue.isConvertibleTo(Json::stringValue)) |
There was a problem hiding this comment.
Do we support both forms index: 3 and index: "3" ?
There was a problem hiding this comment.
More accurately, I think we should.
src/xrpld/rpc/detail/RPCHelpers.cpp
Outdated
| std::uint32_t iVal; | ||
| if (beast::lexicalCastChecked(iVal, index)) | ||
| return getLedger(ledger, iVal, context); | ||
| std::uint32_t iVal; |
There was a problem hiding this comment.
This line wasn't touched by me, I don't see the point in increasing the diff to change a minor variable name
29c7ef9 to
72ed5e2
Compare
72ed5e2 to
91ff59b
Compare
3fb9479 to
b1cf0ce
Compare
b1cf0ce to
99dd5a6
Compare
src/xrpld/rpc/detail/RPCHelpers.cpp
Outdated
| } | ||
|
|
||
| if (hashValue) | ||
| if (not hashValue.isNull()) |
There was a problem hiding this comment.
Not sure if we use this style in rippled but I don't think it blocks anything.
There was a problem hiding this comment.
I believe this was suggested by @godexsoft or @mathbunnyru - it seemed weird to me too
|
Generally looks good. I'll double check functions in LedgerEntry.cpp tomorrow. |
High Level Overview of Change
This PR is a major refactor of
LedgerEntry.cpp. It adds a number of helper functions to make the code easier to maintain.It also splits up the
ledgerandledger_entrytests into different files, and cleans up theledger_entrytests to make them easier to write and maintain.This refactor also caught a few bugs in some of the other RPC processing, so those are fixed along the way.
Context of Change
The
ledger_entrycode wasn't very readable and was hard to extend whenever a new ledger type was created.Type of Change
API Impact
There are some error code changes. They are made as minimal as possible. The test changes should make it clear what the error code changes are.
Notes for Reviewers
The code has been cleaned up into 4 commits. Reviewing will probably be easier if these commits are reviewed separately, as the overall diff for the tests is very difficult to follow.
ledgerandledger_entrytests are split up, and theledger_entrytests are all moved toLedgerEntry_test.cpp. No actual code changes, just moving code around.LedgerEntry.cppare sorted in alphabetical order by ledger entry name. No actual code changes, just moving code around.Test Plan
Tests pass and have better coverage.