Allow account_objects RPC to filter by "check" (RIPD-1589) #2356

scottschurr · 2018-01-25T20:46:13Z

@mDuo13 found a bug in the Checks implementation (Thanks!). This fixes that particular bug. Also adds some more unit tests for account_objects.

ripplelabs-jenkins · 2018-01-25T21:22:01Z

Jenkins Build Summary

Built from this commit

Built at 20180203 - 00:52:48

Test Results

Build Type	Result	Status
clang.debug.unity	986 cases, 0 failed, t: 443s	PASS ✅
coverage	986 cases, 0 failed, t: 573s	PASS ✅
clang.debug.nounity	984 cases, 0 failed, t: 353s	PASS ✅
gcc.debug.unity	986 cases, 0 failed, t: 396s	PASS ✅
gcc.debug.nounity	984 cases, 0 failed, t: 276s	PASS ✅
clang.release.unity	985 cases, 0 failed, t: 470s	PASS ✅
gcc.release.unity	985 cases, 0 failed, t: 510s	PASS ✅

mDuo13 · 2018-01-26T01:53:11Z

(For tracking purposes, this fixes #2350)

mDuo13 · 2018-01-26T01:54:50Z

src/test/rpc/AccountObjects_test.cpp

+
+        // Verify that the non-returning types still don't return anything.
+        // Note that, interestingly, they now return a null object rather
+        // than an empty array.  Not sure what triggers that...


This null object thing might be worth looking into

Agreed. Surprised me a bit too (which is why I called it out). But I have other stuff I need to chase down before I look into this one. My inclination would be to change it so it always return an empty array. But there might be a downside for users who expect a null object to be returned in this case. I'll let you think about that. If we (conservatively) decide not to change the behavior then it's not really worth investigating...

sublimator · 2018-01-26T01:59:02Z

src/test/rpc/AccountObjects_test.cpp

+            [&env, &acct_objs] (Account const& acct, char const* type)
+        {
+            Json::Value const resp = acct_objs (acct, type);
+            return resp["result"]["account_objects"].isNull();


What is the rest of the response? status? Is there an error?

sublimator · 2018-01-26T02:00:05Z

src/test/rpc/AccountObjects_test.cpp

+        // Verify that the non-returning types still don't return anything.
+        // Note that, interestingly, they now return a null object rather
+        // than an empty array.  Not sure what triggers that...
+        BEAST_EXPECT (acct_objs_is_null (gw, jss::account));


These are essentially jibberish, unrecognized types ?

Nevermind, I see. I thought amendments and hashes was non sensical in the context of account_objects. So non-returning types means there was just no results?

I guess it's this line here:
https://github.com/ripple/rippled/blob/develop/src/ripple/rpc/impl/RPCHelpers.cpp#L100

It is probably the weird json library, which creates a null node, until you run append on it.

This is probably the "fix":

auto& jvObjects = jvResult[jss::account_objects] = Json::Value(Json::objectValue);

bachase

Looks good.

I recommend making the change @sublimator suggested; you can cherry-pick bachase@8cff40e.

scottschurr · 2018-01-29T19:14:48Z

Thanks @bachase. Your [FOLD] is incorporated.

miguelportilla · 2018-02-01T20:44:49Z

src/test/rpc/AccountObjects_test.cpp

+
+            bool const isArray = resp["result"]["account_objects"].isArray();
+            return isArray && (resp["result"]["account_objects"].size() == 0);
+        };


Suggestion: If the lambda takes a size param, it can be called from other instances in the test.

auto acct_objs_is_size = [&env, &acct_objs](Account const& acct, char const* type, unsigned size) { Json::Value const resp = acct_objs(acct, type); return resp[jss::result][jss::account_objects].isArray() && resp[jss::result][jss::account_objects].size() == size; };

These instances

BEAST_EXPECT (resp["result"]["account_objects"].isArray()); BEAST_EXPECT (resp["result"]["account_objects"].size() == 1);

Can become

BEAST_EXPECT (acct_objs_is_size(gw, jss::state, 1));

intelliot

Most of this is over my head, but I didn't spot any problems.

intelliot · 2018-02-01T20:17:42Z

src/test/rpc/AccountObjects_test.cpp

@@ -316,10 +316,215 @@ class AccountObjects_test : public beast::unit_test::suite
        }
    }

+    void testObjectTypes()
+    {
+        testcase ("object types");


Looks like (elsewhere in the file) there usually isn't a space between testcase and (?

i.e.

testcase("object types");

intelliot · 2018-02-01T20:18:02Z

src/ripple/rpc/impl/RPCHelpers.cpp

        { {
            { jss::account,         ltACCOUNT_ROOT },
            { jss::amendments,      ltAMENDMENTS },
+            { jss::check,           ltCHECK },


This is just adding 1 new type (check) and reordering a few others?

miguelportilla · 2018-02-01T21:00:16Z

src/test/rpc/AccountObjects_test.cpp

+            auto const& escrow = resp["result"]["account_objects"][0u];
+            BEAST_EXPECT (escrow["Account"] == gw.human());
+            BEAST_EXPECT (escrow["Destination"] == gw.human());
+            BEAST_EXPECT (escrow["Amount"].asInt() == 100'000'000);


Consider asUInt()?

miguelportilla · 2018-02-01T21:08:14Z

src/test/rpc/AccountObjects_test.cpp

+            // Find the trustline and make sure it's the right one.
+            Json::Value const resp = acct_objs (gw, jss::state);
+
+            BEAST_EXPECT (resp["result"]["account_objects"].isArray());


Consider using defined json fields instead.
eg. resp[jss::result][jss::account_objects]

miguelportilla · 2018-02-01T21:15:13Z

src/test/rpc/AccountObjects_test.cpp

+        }
+        // Run up the number of directory entries so gw has two
+        // directory nodes.
+        for (int d = 1'000'032; d >= 1'000'000; d -= 1)


miguelportilla

Left a few suggestions but not holding back. Excellent work @scottschurr 👍

scottschurr · 2018-02-02T00:54:56Z

Thanks for the review, @miguelportilla and @intelliot. Great suggestions. I believe the most recent commit (e8396a2) addresses your comments. If you're good with it I'll mark the pull request passed. Thanks.

ximinez · 2018-02-02T22:31:33Z

(For tracking purposes, this fixes #2350)

If this fixes an issue, you should update the commit message to reflect that. Just include the string "fixes #2350" anywhere in the message, and the issue will get automatically closed when the PR is merged.

Fixes XRPLF#2350 issue on GitHub.

scottschurr · 2018-02-02T23:56:19Z

@ximinez: Here I got trained on how to handle RIPD comments and now you want me to adapt to GitHub issues too. 😉 Commit message updated.

ximinez · 2018-02-03T00:00:36Z

It never ends!

ximinez

👍

ximinez · 2018-02-03T00:08:27Z

src/test/rpc/AccountObjects_test.cpp

+        BEAST_EXPECT (acct_objs_is_size (acct_objs (gw, jss::amendments), 0));
+        BEAST_EXPECT (acct_objs_is_size (acct_objs (gw, jss::directory), 0));
+        BEAST_EXPECT (acct_objs_is_size (acct_objs (gw, jss::fee), 0));
+        BEAST_EXPECT (acct_objs_is_size (acct_objs (gw, jss::hashes), 0));


These tests are great. I'm was going to suggest you do one more unfiltered, but it looks like testUnsteppedThenStepped has that covered.

bachase · 2018-02-05T17:01:13Z

Merged in 88570df

scottschurr assigned miguelportilla and ximinez Jan 25, 2018

scottschurr requested review from miguelportilla and ximinez January 25, 2018 20:46

mDuo13 reviewed Jan 26, 2018

View reviewed changes

mDuo13 mentioned this pull request Jan 26, 2018

Checks (Reference changes) XRPLF/xrpl-dev-portal#296

Merged

sublimator reviewed Jan 26, 2018

View reviewed changes

bachase self-requested a review January 29, 2018 18:15

bachase self-assigned this Jan 29, 2018

bachase approved these changes Jan 29, 2018

View reviewed changes

miguelportilla reviewed Feb 1, 2018

View reviewed changes

intelliot approved these changes Feb 1, 2018

View reviewed changes

miguelportilla reviewed Feb 1, 2018

View reviewed changes

miguelportilla approved these changes Feb 1, 2018

View reviewed changes

scottschurr force-pushed the account-objects-checks branch from f1ed64c to e8396a2 Compare February 2, 2018 00:51

scottschurr unassigned ximinez Feb 2, 2018

scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 2, 2018

scottschurr and others added 3 commits February 2, 2018 14:36

Allow account_objects RPC to filter by "check" (RIPD-1589):

3eba600

Fixes XRPLF#2350 issue on GitHub.

[FOLD] Initialize account_objects as json array

5f922bf

[FOLD] Address review comments

ff81627

scottschurr force-pushed the account-objects-checks branch from e8396a2 to ff81627 Compare February 2, 2018 23:55

ximinez approved these changes Feb 3, 2018

View reviewed changes

bachase mentioned this pull request Feb 3, 2018

Proposed 0.90.0-b6 #2370

Merged

bachase closed this Feb 5, 2018

scottschurr deleted the account-objects-checks branch February 17, 2018 00:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow account_objects RPC to filter by "check" (RIPD-1589) #2356

Allow account_objects RPC to filter by "check" (RIPD-1589) #2356

scottschurr commented Jan 25, 2018

ripplelabs-jenkins commented Jan 25, 2018 •

edited

Loading

mDuo13 commented Jan 26, 2018

mDuo13 Jan 26, 2018

scottschurr Jan 26, 2018

sublimator Jan 26, 2018

sublimator Jan 26, 2018

sublimator Jan 26, 2018

sublimator Jan 26, 2018

bachase left a comment

scottschurr commented Jan 29, 2018

miguelportilla Feb 1, 2018 •

edited

Loading

intelliot left a comment

intelliot Feb 1, 2018

intelliot Feb 1, 2018

miguelportilla Feb 1, 2018 •

edited

Loading

miguelportilla Feb 1, 2018 •

edited

Loading

miguelportilla Feb 1, 2018

miguelportilla left a comment

scottschurr commented Feb 2, 2018

ximinez commented Feb 2, 2018

scottschurr commented Feb 2, 2018

ximinez commented Feb 3, 2018

ximinez left a comment

ximinez Feb 3, 2018

bachase commented Feb 5, 2018

Allow account_objects RPC to filter by "check" (RIPD-1589) #2356

Allow account_objects RPC to filter by "check" (RIPD-1589) #2356

Conversation

scottschurr commented Jan 25, 2018

ripplelabs-jenkins commented Jan 25, 2018 • edited Loading

Jenkins Build Summary

Test Results

mDuo13 commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase left a comment

Choose a reason for hiding this comment

scottschurr commented Jan 29, 2018

miguelportilla Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

intelliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

miguelportilla Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla left a comment

Choose a reason for hiding this comment

scottschurr commented Feb 2, 2018

ximinez commented Feb 2, 2018

scottschurr commented Feb 2, 2018

ximinez commented Feb 3, 2018

ximinez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase commented Feb 5, 2018

ripplelabs-jenkins commented Jan 25, 2018 •

edited

Loading

miguelportilla Feb 1, 2018 •

edited

Loading

miguelportilla Feb 1, 2018 •

edited

Loading

miguelportilla Feb 1, 2018 •

edited

Loading