[FOLD] Review feedback from @scottschurr:

* Add more NFTokenOffer object test cases * Add comments explaining the method for checking sfDestination
XRPLF · Dec 14, 2022 · e9f358f · e9f358f
1 parent 2756540
commit e9f358f
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 11 deletions.
diff --git a/src/ripple/rpc/impl/RPCHelpers.cpp b/src/ripple/rpc/impl/RPCHelpers.cpp
@@ -122,6 +122,12 @@ isRelatedToAccount(
     }
     else if (sle->isFieldPresent(sfAccount))
     {
+        // If there's an sfAccount present, also test the sfDestination, if
+        // present. This will match objects such as Escrows (ltESCROW), Payment
+        // Channels (ltPAYCHAN), and Checks (ltCHECK) because those are added to
+        // the Destination account's directory. It intentionally EXCLUDES
+        // NFToken Offers (ltNFTOKEN_OFFER). NFToken Offers are NOT added to the
+        // Destination account's directory.
         return sle->getAccountID(sfAccount) == accountID ||
             (sle->isFieldPresent(sfDestination) &&
              sle->getAccountID(sfDestination) == accountID);
@@ -133,6 +139,8 @@ isRelatedToAccount(
     }
     else if (sle->getType() == ltNFTOKEN_OFFER)
     {
+        // Do not check the sfDestination field. NFToken Offers are NOT added to
+        // the Destination account's directory.
         return sle->getAccountID(sfOwner) == accountID;
     }
 

diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLinesRPC_test.cpp
@@ -618,6 +618,25 @@ class AccountLinesRPC_test : public beast::unit_test::suite
         env(token::createOffer(becky, aliceNFtokenID, drops(1)),
             token::owner(alice));
 
+        env(token::createOffer(becky, beckyNFtokenID, drops(1)),
+            txflags(tfSellNFToken),
+            token::destination(alice));
+        env(token::createOffer(alice, aliceNFtokenID, drops(1)),
+            txflags(tfSellNFToken),
+            token::destination(becky));
+
+        env(token::createOffer(gw1, beckyNFtokenID, drops(1)),
+            token::owner(becky),
+            token::destination(alice));
+        env(token::createOffer(gw1, aliceNFtokenID, drops(1)),
+            token::owner(alice),
+            token::destination(becky));
+
+        env(token::createOffer(becky, beckyNFtokenID, drops(1)),
+            txflags(tfSellNFToken));
+        env(token::createOffer(alice, aliceNFtokenID, drops(1)),
+            txflags(tfSellNFToken));
+
         // Checks, in each direction
         env(check::create(alice, becky, XRP(50)));
         env(check::create(becky, alice, XRP(50)));
@@ -645,11 +664,21 @@ class AccountLinesRPC_test : public beast::unit_test::suite
             // Now make repeated calls to `account_lines` with a limit of 1.
             // That should iterate all of alice's relevant objects, even though
             // the list will be empty for most calls.
-            auto aliceLines = env.rpc(
-                "json",
-                "account_lines",
-                R"({"account": ")" + alice.human() + R"(", "limit": 1})");
-            std::size_t const expectedLines = 2;
+            auto getNextLine = [](Env& env,
+                                  Account const& alice,
+                                  std::optional<std::string> const marker) {
+                Json::Value params(Json::objectValue);
+                params[jss::account] = alice.human();
+                params[jss::limit] = 1;
+                if (marker)
+                    params[jss::marker] = *marker;
+
+                return env.rpc("json", "account_lines", to_string(params));
+            };
+
+            auto aliceLines = getNextLine(env, alice, std::nullopt);
+            constexpr std::size_t expectedIterations = 16;
+            constexpr std::size_t expectedLines = 2;
             std::size_t foundLines = 0;
 
             auto hasMarker = [](auto const& aliceLines) {
@@ -675,11 +704,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite
             while (hasMarker(aliceLines))
             {
                 // Iterate through the markers
-                aliceLines = env.rpc(
-                    "json",
-                    "account_lines",
-                    R"({"account": ")" + alice.human() + R"(", "marker": ")" +
-                        marker(aliceLines) + R"(", "limit": 1})");
+                aliceLines = getNextLine(env, alice, marker(aliceLines));
                 BEAST_EXPECT(checkLines(aliceLines));
                 foundLines += aliceLines[jss::result][jss::lines].size();
                 ++iterations;
@@ -707,7 +732,28 @@ class AccountLinesRPC_test : public beast::unit_test::suite
                 iterations);
             // If ledger object association ever changes, for whatever
             // reason, this test will need to be updated.
-            BEAST_EXPECTS(iterations == 14, std::to_string(iterations));
+            BEAST_EXPECTS(
+                iterations == expectedIterations, std::to_string(iterations));
+
+            // Get becky's objects just to confirm that they're symmetrical
+            Json::Value const beckyObjects = env.rpc(
+                "json",
+                "account_objects",
+                R"({"account": ")" + becky.human() +
+                    R"(", )"
+                    R"("limit": 200})");
+            BEAST_EXPECT(beckyObjects.isMember(jss::result));
+            BEAST_EXPECT(
+                !beckyObjects[jss::result].isMember(jss::error_message));
+            BEAST_EXPECT(
+                beckyObjects[jss::result].isMember(jss::account_objects));
+            BEAST_EXPECT(
+                beckyObjects[jss::result][jss::account_objects].isArray());
+            // becky should have the same number of objects as alice, except the
+            // 2 tickets that only alice created.
+            BEAST_EXPECT(
+                beckyObjects[jss::result][jss::account_objects].size() ==
+                aliceObjects[jss::result][jss::account_objects].size() - 2);
         }
     }