Added public_key to ValidatorList::getAvailable response #3488

natenichols · 2020-07-06T15:06:13Z

High Level Overview of Change

Public key from the /vl/ request URL is now copied back in rippled's response.

Context of Change

API change Fixes #3392, vl method should return public key (Version: 1.5.0). The response from rippled at the endpoint /vl/<public_key> now includes the public_key.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

Before / After

Request: https://localhost:51235/vl/ED2677ABFFD1B33AC6FBC3062B71F1E8397C1505E1C42C64D11AD1B28FF73F4734
Response:
Before
{ "blob": "eyJzZ...J9XX0=", "manifest": "JAAAA....sAg==", "signature": "4C59F...883E03", "version": 1 }
After
{ "blob": "eyJzZ...J9XX0=", "manifest": "JAAAA....sAg==", "public_key": "ED2677ABFFD1B33AC6FBC3062B71F1E8397C1505E1C42C64D11AD1B28FF73F4734", "signature": "4C59F...883E03", "version": 1 }

Test Plan

None currently, and am open to suggestions for how to go about writing tests.

ximinez

Looks great! Thanks for your contribution! 👍

Edit: I notice that Travis CI is failing, but I think that's an ongoing issue with Travis, not this PR.

mDuo13

Looks awesome to me!

scottschurr

Your change looks good. However I'd like to request two changes.

The area where you made your change has pre-existing bugs. I'm hoping you will fix those bugs while you're in the area. The comments should give you a pretty good idea of what to do. But you can ask me if you have questions.
The somewhat bigger ask is that we shouldn't make this change without also including unit test coverage for it. I'm really not familiar with the unit tests for ValidatorList. But I found the file that has the tests. It's here: https://github.com/ripple/rippled/blob/develop/src/test/app/ValidatorList_test.cpp

I'm not a good choice to help you add the unit test support, however. I suggest that you ask @ximinez for some help, since I think he knows these tests pretty well.

As the saying goes, no good deed goes unpunished. Thanks for the submission. Sorry it's not going as smoothly as it might.

scottschurr · 2020-07-08T01:29:54Z

src/ripple/app/misc/impl/ValidatorList.cpp

@@ -820,6 +820,7 @@ ValidatorList::getAvailable(boost::beast::string_view const& pubKey)

    Json::Value value(Json::objectValue);

+    value["public_key"] = std::string{pubKey};


This change looks great, as far as it goes. However the area that this change is in has some bugs. Rather than let the bugs spread I'd like you to squash these bugs while you are in the vicinity.

Our interfaces are set up so that programmers can use literal strings to supply parameters to our interfaces. But literal strings are error prone and subject to things like typos. So we have places that we like to get common user interface strings from. This helps to assure that the always spell (and capitalize) strings in our interfaces the same way.

So, in place of "public_key" I'd like you to use jss::public_key.

That same sort of change needs to also be made for the four other near-by quoted strings. So the final result will look something like...

value[jss::public_key] = std::string{pubKey}; value[jss::manifest] = iter->second.rawManifest; value[jss::blob] = iter->second.rawBlob; value[jss::signature] = iter->second.rawSignature; value[jss::version] = iter->second.rawVersion;

Once you've done this you'll get a compile error. It turns out that a jss string (jss stands for JSON Static String) has not yet been defined for "blob". So you'll need to add that. To do so navigate to here:

https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/jss.h#L146

This file contains all of the jss strings in alphabetical order. You'll add one for "blob" right above the line that says JSS(books);. The final result should look something like this:

JSS(blob); // out: ValidatorList JSS(books); // in: Subscribe, Unsubscribe

Then, assuming everything builds and passes unit tests, I think we're good.

I've changed the strings to jss and added "blob" to jss.h. I'll talk to @ximinez about adding Validator_List unit tests for this PR.

Thanks Scott!

ximinez

Nothing major. The added test is great!

ximinez · 2020-07-13T15:35:23Z

src/test/app/ValidatorList_test.cpp

+        BEAST_EXPECT(available);
+        if (available)


BEAST_EXPECT returns the bool value of the test, so you can write this as if (BEAST_EXPECT(available))

ximinez · 2020-07-13T15:40:47Z

src/test/app/ValidatorList_test.cpp

+            BEAST_EXPECT(
+                available->get(jss::public_key, Json::nullValue) == hexPublic);
+            BEAST_EXPECT(available->get(jss::blob, Json::nullValue) == blob2);
+            BEAST_EXPECT(
+                available->get(jss::manifest, Json::nullValue) == manifest1);
+            BEAST_EXPECT(
+                available->get(jss::version, Json::nullValue) == version);
+            BEAST_EXPECT(
+                available->get(jss::signature, Json::nullValue) == sig2);


get is fine here, but IMHO it's a little easier to read calls using the operator[], mainly because, that default value clutters things up. Since available is an optional, you can dereference it into a local to make things prettier:

auto const& a = *available; BEAST_EXPECT(a[jss::public_key] == hexPublic); BEAST_EXPECT(a[jss::blob] == blob2); BEAST_EXPECT(a[jss::manifest] == manifest1); BEAST_EXPECT(a[jss::version] == version); BEAST_EXPECT(a[jss::signature] == sig2);

This is more personal preference, so whether to change it is up to you.

ximinez · 2020-07-13T17:04:55Z

src/test/app/ValidatorList_test.cpp

+        BEAST_EXPECT(available);
+        if (available)
+        {
+            BEAST_EXPECT(


You're missing an include for the non-unity builds, so jss is unknown. You need #include <ripple/protocol/jss.h>

ximinez

Looks great!

scottschurr

👍 Nice! It all looks good to me.

FWIW, I looked at the code coverage after adding your unit test. ValidatorList::getAvailable() was previously untouched by unit tests. Your added test now exercises 23 of the 29 lines in the method, including the line you added. Nice improvement in coverage. Thanks.

nbougalis

👍

Fixes XRPLF#924. Documents XRPLF/rippled#3488

Added public_key to ValidatorList::getAvailable response

91704a4

natenichols requested review from nbougalis, mDuo13 and scottschurr July 6, 2020 15:06

ximinez self-requested a review July 6, 2020 16:35

ximinez self-assigned this Jul 6, 2020

ximinez approved these changes Jul 6, 2020

View reviewed changes

mDuo13 approved these changes Jul 8, 2020

View reviewed changes

scottschurr assigned nbougalis, mDuo13 and scottschurr Jul 8, 2020

scottschurr suggested changes Jul 8, 2020

View reviewed changes

natenichols added 3 commits July 7, 2020 23:06

changed string literals to JSON static strings

1fb4ef7

changed string literals to JSON static strings

52446e3

removed whitespace so clang-format will pass

1035d56

ximinez self-requested a review July 9, 2020 23:25

natenichols added 3 commits July 11, 2020 17:12

added unit tests for ValidatorList::getAvailable

9c7c0a5

added unit tests for ValidatorList::getAvailable

febeeeb

formatted tests

bc43f0d

natenichols requested a review from scottschurr July 11, 2020 22:25

ximinez requested changes Jul 13, 2020

View reviewed changes

natenichols added 2 commits July 13, 2020 13:34

changed test formatting

e0fdf03

ran clang-format

8a63ff3

natenichols requested a review from ximinez July 13, 2020 18:39

ximinez approved these changes Jul 13, 2020

View reviewed changes

scottschurr approved these changes Jul 14, 2020

View reviewed changes

nbougalis approved these changes Jul 30, 2020

View reviewed changes

ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 30, 2020

intelliot approved these changes Aug 20, 2020

View reviewed changes

nbougalis mentioned this pull request Sep 1, 2020

Proposed 1.7.0-b1 #3599

Merged

nbougalis closed this in #3599 Sep 8, 2020

mDuo13 mentioned this pull request Nov 3, 2020

Document public_key field in /vl/ method XRPLF/xrpl-dev-portal#924

Closed

mDuo13 added a commit to mDuo13/xrpl-dev-portal that referenced this pull request Feb 23, 2021

vl method: add public_key field

de7485d

Fixes XRPLF#924. Documents XRPLF/rippled#3488

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added public_key to ValidatorList::getAvailable response #3488

Added public_key to ValidatorList::getAvailable response #3488

natenichols commented Jul 6, 2020 •

edited

Loading

ximinez left a comment •

edited

Loading

mDuo13 left a comment

scottschurr left a comment

scottschurr Jul 8, 2020

natenichols Jul 8, 2020

ximinez left a comment

ximinez Jul 13, 2020

ximinez Jul 13, 2020

ximinez Jul 13, 2020

ximinez left a comment

scottschurr left a comment

nbougalis left a comment

		@@ -820,6 +820,7 @@ ValidatorList::getAvailable(boost::beast::string_view const& pubKey)

		Json::Value value(Json::objectValue);

		value["public_key"] = std::string{pubKey};

Added public_key to ValidatorList::getAvailable response #3488

Added public_key to ValidatorList::getAvailable response #3488

Conversation

natenichols commented Jul 6, 2020 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

Before / After

Test Plan

ximinez left a comment • edited Loading

Choose a reason for hiding this comment

mDuo13 left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Jul 8, 2020

Choose a reason for hiding this comment

natenichols Jul 8, 2020

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

ximinez Jul 13, 2020

Choose a reason for hiding this comment

ximinez Jul 13, 2020

Choose a reason for hiding this comment

ximinez Jul 13, 2020

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

natenichols commented Jul 6, 2020 •

edited

Loading

ximinez left a comment •

edited

Loading