feat: add user version of `feature` RPC

[mvadari]

High Level Overview of Change

This PR modifies the feature RPC to have a non-admin mode that allows conversion from the hex encodings of amendments to human-readable strings.

Example:

> {
    "command": "feature",
    "feature": "2E2FB9CF8A44EB80F4694D38AADAE9B8B7ADAFD2F092E10068E61C98C4F092B0"
  }
< {
    "result": {
      "2E2FB9CF8A44EB80F4694D38AADAE9B8B7ADAFD2F092E10068E61C98C4F092B0": {
        "enabled": false,
        "name": "fixUniversalNumber",
        "supported": true
      }
    },
    "type": "response"
  }

Context of Change

This has been requested by many explorer operators, so that the on-ledger hex amendment names can be decoded.

Type of Change

• [x] New feature (non-breaking change which adds functionality)

API Impact

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

Test Plan

Added tests to handle this case.

[JST5000]

As an additional note, this is a feature I tried to find when I had when trying to set up our docker container for xrpl.js and xrpl-py CI since the Amendments stanza in the rippled.cfg file to force-enable amendments is really hard to read without the human-readable versions of amendments.

[dangell7]

This seems overkill.

import hashlib

def hash_string(k: str) -> str:
    return hashlib.sha512(k.encode('utf-8')).hexdigest().upper()[:64]

import * as crypto from 'crypto';

function sha512Hash(k: string): string {
  return crypto.createHash('sha512').update(k, 'utf8').digest('hex').toUpperCase().substring(0, 64);
}

[mvadari]

This seems overkill.

This works for going from the name to the hex, but not the other way around, since you can't reverse a hash. That's where this is needed, because that's what the ledger stores.

[pkcs8]

Would the non-admin version of this command have similar response shape as the admin version? I.e., if I skip the feature property, and just send {"command":"feature"}, it should return a map of all amendments known to this rippled sans voting data.

[mvadari]

Would the non-admin version of this command have similar response shape as the admin version? I.e., if I skip the feature property, and just send {"command":"feature"}, it should return a map of all amendments known to this rippled sans voting data.

I didn't add that here because it's a bit harder to do. But if that's a need, I can add it.

[pkcs8]

I read rippled websocket url from the environment, and often switch and use admin and non-admin ports interchangeably. I'm not sure now complicated it would be, but would be great if response shape remains same so I don't have to write conditions. I like how account_objects command works with its type filter.

[shortthefomo]

I didn't add that here because it's a bit

I'm actually a +1 for this I need a way to figure the list of amendments on the network. Currently using external API for mainnet or hardcoded. Would help a lot.

[mvadari]

@pkcs8 @lathanbritz I have switched to the existing admin feature format (removing the admin fields of course) and added the ability to get a list of all features in https://github.com/XRPLF/rippled/pull/4781/commits/43a1ce7b1516b93d3abbf034050afa27c641c956.

[mvadari]

@intelliot can I add @ximinez and @thejohnfreeman as reviewers (if they're okay with that)?

[intelliot]

@mvadari I requested ximinez and thejohnfreeman but the PRs listed here are higher priority: https://github.com/XRPLF/rippled/milestone/8 <- these are 2.0 release blockers

This PR (#4781) is currently planned for 2.1.0

[dangell7]

We don't like this on feature endpoint. We believe opening up feature like this is dangerous. We have opted to move this to the ServerDefinitions.cpp file.

What are your thoughts on this?

I know this goes against what XRPScan asked for but in our opinion its a "safer" solution.

[mvadari]

We don't like this on feature endpoint. We believe opening up feature like this is dangerous. We have opted to move this to the ServerDefinitions.cpp file.

What are your thoughts on this?

I know this goes against what XRPScan asked for but in our opinion its a "safer" solution.

This code change is pretty small and therefore easy to audit and test from a security perspective. There is also prior art for functions that have separate user versions and admin versions, like server_info, and there have been no issues with those. I'm personally not concerned, but there's also a reason we go through code review 🙂

[intelliot]

Perf Test Desired (Optional): This is a new feature in the API, but the perf impact should be negligible.

Internal tracker: RPFC-109

[intelliot]

I think this can be merged once the (trivial) merge conflicts are resolved

[mvadari]

@intelliot merge conflicts have been resolved and I believe this is ready to be merged.

[mvadari]

The coverage check failed to upload, but I don't have permissions to re-run it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 61.65%. Comparing base (cce09b7) to head (df55811).

Files	Patch %	Lines
src/ripple/app/misc/impl/AmendmentTable.cpp	66.66%	0 Missing and 2 partials :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4781      +/-   ##
===========================================
+ Coverage    61.63%   61.65%   +0.02%     
===========================================
  Files          806      806              
  Lines        70962    70967       +5     
  Branches     36686    36690       +4     
===========================================
+ Hits         43740    43758      +18     
+ Misses       19863    19855       -8     
+ Partials      7359     7354       -5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[intelliot]

@ximinez or @Bronek or @mvadari - could you have a quick look to see if the "2 lines in your changes are missing coverage" would be easy to address?

I'm ok to merge as-is if that's what y'all want, but let's confirm that's what we want to do (rather than adding some coverage first)

[Bronek]

@ximinez or @Bronek or @mvadari - could you have a quick look to see if the "2 lines in your changes are missing coverage" would be easy to address?

I'm ok to merge as-is if that's what y'all want, but let's confirm that's what we want to do (rather than adding some coverage first)

This is referring lines 917 and 926 of AmendmentTable.cpp. There's a newly added isAdmin parameter which is used in branching instructions in these lines. Looking at AmendmentTable_test.cpp this parameter seem to be only set to true, but I do not know about other tests. I think it should be easy to add unit tests to cover isAdmin=false, too.

[Bronek]

@ximinez or @Bronek or @mvadari - could you have a quick look to see if the "2 lines in your changes are missing coverage" would be easy to address? I'm ok to merge as-is if that's what y'all want, but let's confirm that's what we want to do (rather than adding some coverage first)

This is referring lines 917 and 926 of AmendmentTable.cpp. There's a newly added isAdmin parameter which is used in branching instructions in these lines. Looking at AmendmentTable_test.cpp this parameter seem to be only set to true, but I do not know about other tests. I think it should be easy to add unit tests to cover isAdmin=false, too.

I looked at test coverage report of the develop branch and see similar partial branch coverage remarks

This makes me think that the coverage warning is not new actually, and also that it refers to fs.enabled and not isAdmin.

[intelliot]

• @mvadari to confirm whether she thinks, given the above, this is still ready to be merged now

[mvadari]

@mvadari to confirm whether she thinks, given the above, this is still ready to be merged now

I checked the tests and I'm pretty sure both branches of isAdmin are already checked (cc @Bronek to double check since he's already taken a look at some of the code). So yes, I think that this is ready for merge.

[intelliot]

(cc @Bronek to double check since he's already taken a look at some of the code)

[Bronek]

(cc @Bronek to double check since he's already taken a look at some of the code)

Thank you @intelliot ; I looked some more and there is one thing that can be improved in unit tests; added it in my feedback.

I use cmake -Dcoverage-format=json-details -Dcoverage=ON . . . to prepare coverage.json report and see from it that the proposed addition will increase branch coverage of line 929 in AmendmentTable.cpp from 6/8 to 7/8 .

[ximinez]

@mvadari Is this ready to merge?

[mvadari]

@mvadari Is this ready to merge?

@ximinez with Bronek's ✅ , yep - I think it was just waiting on the 2.1 release.

[ximinez]

[see following comment]

[mvadari]

Proposed commit message:

feat: add user version of `feature` RPC (#4781)

* hides potentially sensitive data
* uses same formatting as admin RPC

@ximinez nit: I'd switch the order of the bullets

feat: add user version of `feature` RPC (#4781):

* uses same formatting as admin RPC
* hides potentially sensitive data

[intelliot]

From my perspective, @ximinez or @seelabs are welcome to merge this. (no big rush though)