Add ledger_entry options for fee, amendments, NUNL, and hashes

[ximinez]

High Level Overview of Change

Adds ledger_entry functionality for global record-keeping objects, with some flexibility.

Also will always return the hash for any valid ledger_entry request, even if the object is not found or valid.

Context of Change

While doing some analysis, I was annoyed that I couldn't look up these records without computing their indexes or finding them on the ledger_entry documentation page.

Type of Change

• [X] New feature (non-breaking change which adds functionality)
• [X] Tests (you added tests for code that already exists, or your new feature included in this PR)

API Impact

• [X] Public API: New feature (new methods and/or new fields)

Before / After

Affects the Amendments, FeeSettings, NegativeUNL, and LedgerHashes objects.

Before:

These objects could only be found using, for example, "index" : "7DB0788C020F02780A673DC74757F23823FA3014C1866E72CC4CD8B226CD6EF4" for Amendments.

Additionally, if an object is not found, only the error is returned.

After:

These objects can be found in multiple ways:

1. Using the format of other ledger objects, like accounts, but the only valid value is true or the valid index of the object: "amendments" : true. e.g. "amendments" : true.
   1. Amendments: "amendments" : true
   2. FeeSettings: "fee" : true
   3. NegativeUNL: "nunl" : true
   4. LedgerHashes: "hashes" : true (For the "short" list. See below.)
2. Using special case values to "index", such as "index" : "amendments". Uses the same names as above. Note that for "hashes", this option will only return the recent ledger hashes / "short" skip list.
3. LedgerHashes has two types: "short", which stores the recent hashes since the last flag ledger, and "long", which stores the flag ledger hashes for a particular ledger range.
   1. To find a "long" LedgerHashes object, request "hashes" : <ledger sequence>. <ledger sequence> can be integer value that evaluates to an unsigned integer.
   2. To find the "short" LedgerHashes object, request "hashes": true as with the other fixed types.

The following queries are all functionally equivalent:

• "amendments" : true
• "index" : "amendments"
• "amendments" : "7DB0788C020F02780A673DC74757F23823FA3014C1866E72CC4CD8B226CD6EF4"
• "index" : "7DB0788C020F02780A673DC74757F23823FA3014C1866E72CC4CD8B226CD6EF4"

Finally, whether the object is found or not, if the query results in a valid index, that index will be returned. This can be used to confirm the query was valid, or to save the index for future use.

[mvadari]

Some thoughts on the fixed ledger entries:

• Should the value be truthy?
• If a string hash is provided, shouldn't that be checked for correctness?

[ximinez]

Some thoughts on the fixed ledger entries:

* Should the value be truthy?

* If a string hash is provided, shouldn't that be checked for correctness?

@mvadari Could you clarify what you mean for both of these questions? Maybe with examples? Or the updated description may answer them.

[mvadari]

• Should the value be truthy?

Instead of accepting all bools provided into the amendments field, only accept true. Or don't accept 0 or negative numbers, only positive numbers. I thought the concept of "truthiness" was a general programming thing, but it seems to only be in JS: https://www.geeksforgeeks.org/javascript/explain-the-concept-of-truthy-falsy-values-in-javascript/

Edit after reading the description: I think passing in null and expecting not-an-error to be returned is rather confusing.

• If a string hash is provided, shouldn't that be checked for correctness?

You can provide amendments: "01234...". Shouldn't that string be required to only be the actual fixed key, rather than accepting any value?

[codecov[bot]]

Codecov Report

:x: Patch coverage is 87.71930% with 7 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 79.1%. Comparing base (41c1be2) to head (6009636).

Files with missing lines	Patch %	Lines
src/xrpld/rpc/handlers/LedgerEntry.cpp	87.7%	7 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5644   +/-   ##
=======================================
  Coverage     79.1%   79.1%           
=======================================
  Files          836     836           
  Lines        71245   71268   +23     
  Branches      8324    8310   -14     
=======================================
+ Hits         56358   56391   +33     
+ Misses       14887   14877   -10     

Files with missing lines	Coverage Δ
src/xrpld/rpc/handlers/LedgerEntry.cpp	79.7% <87.7%> (+3.8%)	:arrow_up:

... and 8 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ximinez]

Thank you for explaining!

My main goal with this change, aside from just getting something I could use myself, was to make it as easy as possible to get the information I wanted. I avoided doing any validation of the input value, because it makes absolutely no difference to the lookup functionality.

• Should the value be truthy?

Instead of accepting all bools provided into the amendments field, only accept true. Or don't accept 0 or negative numbers, only positive numbers. I thought the concept of "truthiness" was a general programming thing, but it seems to only be in JS: geeksforgeeks.org/javascript/explain-the-concept-of-truthy-falsy-values-in-javascript

Edit after reading the description: I think passing in null and expecting not-an-error to be returned is rather confusing.

Ah. I see. I was loosely familiar with "truthiness", but I didn't see how it applied here, since I didn't care about the value, only they key. That was partially informed by the implementation, where the first matched key is the one used for the query. To me it would be nonsensical to request { "account_root" : false, "check" : "ABCD..." }, because it'll only process the first hit (account_root), which will fail, because false is an invalid input. Further, it would be absurd to expect a Check in the result.

Given that, and given that the input doesn't need to be parsed, and given that I want it to be easy, I opted to do no validation of any kind.

It could be added, but my attitude is that nobody is going to complain about not getting an error, right?

• If a string hash is provided, shouldn't that be checked for correctness?

You can provide amendments: "01234...". Shouldn't that string be required to only be the actual fixed key, rather than accepting any value?

Ah. I understand. This question basically has the same answer as the first. Aside from the fact that passing a key would be silly, since you can just use "index":"01234...", if I'm not doing any validation of the input, then they just get the Amendment object back, and if that's not what they expect, they need to write a better request.

[ximinez]

my attitude is that nobody is going to complain about not getting an error, right?

On second thought. I forgot my old maxim that "a wrong answer is worse than no answer". And if someone passes some other index as the value for "amendments":value, expecting the entry at that index, then returning the Amendments object is, arguably, a wrong answer.

I'm rewriting it now so that true is the only valid value for all these queries, with the addition of a "ledger indexy" value for hashes.

Edit: true or an index key.

[mvadari]

my attitude is that nobody is going to complain about not getting an error, right?

On second thought. I forgot my old maxim that "a wrong answer is worse than no answer". And if someone passes some other index as the value for "amendments":value, expecting the entry at that index, then returning the Amendments object is, arguably, a wrong answer.

I'm rewriting it now so that true is the only valid value for all these queries, with the addition of a "ledger indexy" value for hashes.

Edit: true or an index key.

I would perhaps accept all non-null, non-string values (plus the actual key string). null because I hate how Json::Value treats null as a real value and I don't want to support it here as a real value, especially since Json::Value will automatically add a null value if you do obj[key] before checking membership.

[ximinez]

I would perhaps accept all non-null, non-string values (plus the actual key string). null because I hate how Json::Value treats null as a real value and I don't want to support it here as a real value, especially since Json::Value will automatically add a null value if you do obj[key] before checking membership.

I decided to limit it to true or the key string for simplicity, both for the user, and for documentation. Also because a number has meaning to hashes, but it otherwise follows the same rules, so this gives consistency.

[tequdev]

How about supporting amendment: {}? This would be consistent with other non-fixed types.

[mvadari]

I decided to limit it to true or the key string for simplicity, both for the user, and for documentation. Also because a number has meaning to hashes, but it otherwise follows the same rules, so this gives consistency.

This seems fine to me, mostly because it's easier to start narrow and expand than the reverse. Worst case we revisit this in a few months and decide to expand the number of inputs we support here.

[ximinez]

How about supporting amendment: {}? This would be consistent with other non-fixed types.

It wouldn't be all that consistent, IMHO. First json objects are used to pass additional parameters. There are no additional parameters needs here. Second, not all of the other types accept objects. AccountRoot and Check are two examples

[mvadari]

How about supporting amendment: {}? This would be consistent with other non-fixed types.

It wouldn't be all that consistent, IMHO. First json objects are used to pass additional parameters. There are no additional parameters needs here. Second, not all of the other types accept objects. AccountRoot and Check are two examples

I support your reasoning on this decision, but IMO some objects like Check should support objects and it's just a missing feature that it doesn't.