Skip to content

Commit

Permalink
fix "account_nfts" with unassociated marker returning issue (XRPLF#5045)
Browse files Browse the repository at this point in the history 
* fix "account_nfts" with unassociated marker returning issue

* create unit test for fixing nft page invalid marker not returning error

add more test

change test name

create unit test

* fix "account_nfts" with unassociated marker returning issue

* fix "account_nfts" with unassociated marker returning issue

* fix "account_nfts" with unassociated marker returning issue

* fix "account_nfts" with unassociated marker returning issue

* fix "account_nfts" with unassociated marker returning issue

* fix "account_nfts" with unassociated marker returning issue

* fix "account_nfts" with unassociated marker returning issue

* fix "account_nfts" with unassociated marker returning issue

* [FOLD] accumulated review suggestions

* move BEAST check out of lambda function

---------

Authored-by: Scott Schurr <[email protected]>
  • Loading branch information
@yinyiqian1 @vlntb
yinyiqian1 authored and vlntb committed Aug 23, 2024
1 parent eecd343 commit db238bc
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 50 deletions.
161 changes: 117 additions & 44 deletions src/test/rpc/AccountObjects_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
#include <test/jtx.h>
#include <test/jtx/AMM.h>
#include <test/jtx/xchain_bridge.h>
#include <xrpld/app/tx/detail/NFTokenMint.h>
#include <xrpl/json/json_reader.h>
#include <xrpl/json/json_value.h>
#include <xrpl/json/to_string.h>
#include <xrpl/protocol/jss.h>
#include <xrpl/protocol/nft.h>

#include <boost/utility/string_ref.hpp>

Expand Down Expand Up @@ -123,30 +125,8 @@ class AccountObjects_test : public beast::unit_test::suite

// test error on no account
{
Json::Value params;
auto resp = env.rpc("json", "account_objects", to_string(params));
BEAST_EXPECT(
resp[jss::result][jss::error_message] ==
"Missing field 'account'.");
}
// test account non-string
{
auto testInvalidAccountParam = [&](auto const& param) {
Json::Value params;
params[jss::account] = param;
auto jrr = env.rpc(
"json", "account_objects", to_string(params))[jss::result];
BEAST_EXPECT(jrr[jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::error_message] == "Invalid field 'account'.");
};

testInvalidAccountParam(1);
testInvalidAccountParam(1.1);
testInvalidAccountParam(true);
testInvalidAccountParam(Json::Value(Json::nullValue));
testInvalidAccountParam(Json::Value(Json::objectValue));
testInvalidAccountParam(Json::Value(Json::arrayValue));
auto resp = env.rpc("json", "account_objects");
BEAST_EXPECT(resp[jss::error_message] == "Syntax error.");
}
// test error on malformed account string.
{
Expand Down Expand Up @@ -1055,32 +1035,125 @@ class AccountObjects_test : public beast::unit_test::suite
}

void
testAccountNFTs()
testNFTsMarker()
{
testcase("account_nfts");
// there's some bug found in account_nfts method that it did not
// return invalid params when providing unassociated nft marker.
// this test tests both situations when providing valid nft marker
// and unassociated nft marker.
testcase("NFTsMarker");

using namespace jtx;
Env env(*this);

// test validation
Account const bob{"bob"};
env.fund(XRP(10000), bob);

static constexpr unsigned nftsSize = 10;
for (unsigned i = 0; i < nftsSize; i++)
{
auto testInvalidAccountParam = [&](auto const& param) {
Json::Value params;
params[jss::account] = param;
auto jrr = env.rpc(
"json", "account_nfts", to_string(params))[jss::result];
BEAST_EXPECT(jrr[jss::error] == "invalidParams");
BEAST_EXPECT(
jrr[jss::error_message] == "Invalid field 'account'.");
};
env(token::mint(bob, 0));
}

env.close();

testInvalidAccountParam(1);
testInvalidAccountParam(1.1);
testInvalidAccountParam(true);
testInvalidAccountParam(Json::Value(Json::nullValue));
testInvalidAccountParam(Json::Value(Json::objectValue));
testInvalidAccountParam(Json::Value(Json::arrayValue));
// save the NFTokenIDs to use later
std::vector<Json::Value> tokenIDs;
{
Json::Value params;
params[jss::account] = bob.human();
params[jss::ledger_index] = "validated";
Json::Value const resp =
env.rpc("json", "account_nfts", to_string(params));
Json::Value const& nfts = resp[jss::result][jss::account_nfts];
for (Json::Value const& nft : nfts)
tokenIDs.push_back(nft["NFTokenID"]);
}

// this lambda function is used to check if the account_nfts method
// returns the correct token information. lastIndex is used to query the
// last marker.
auto compareNFTs = [&tokenIDs, &env, &bob](
unsigned const limit, unsigned const lastIndex) {
Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = limit;
params[jss::marker] = tokenIDs[lastIndex];
params[jss::ledger_index] = "validated";
Json::Value const resp =
env.rpc("json", "account_nfts", to_string(params));

if (resp[jss::result].isMember(jss::error))
return false;

Json::Value const& nfts = resp[jss::result][jss::account_nfts];
unsigned const nftsCount = tokenIDs.size() - lastIndex - 1 < limit
? tokenIDs.size() - lastIndex - 1
: limit;

if (nfts.size() != nftsCount)
return false;

for (unsigned i = 0; i < nftsCount; i++)
{
if (nfts[i]["NFTokenID"] != tokenIDs[lastIndex + 1 + i])
return false;
}

return true;
};

// test a valid marker which is equal to the third tokenID
BEAST_EXPECT(compareNFTs(4, 2));

// test a valid marker which is equal to the 8th tokenID
BEAST_EXPECT(compareNFTs(4, 7));

// lambda that holds common code for invalid cases.
auto testInvalidMarker = [&env, &bob](
auto marker, char const* errorMessage) {
Json::Value params;
params[jss::account] = bob.human();
params[jss::limit] = 4;
params[jss::ledger_index] = jss::validated;
params[jss::marker] = marker;
Json::Value const resp =
env.rpc("json", "account_nfts", to_string(params));
return resp[jss::result][jss::error_message] == errorMessage;
};

// test an invalid marker that is not a string
BEAST_EXPECT(
testInvalidMarker(17, "Invalid field \'marker\', not string."));

// test an invalid marker that has a non-hex character
BEAST_EXPECT(testInvalidMarker(
"00000000F51DFC2A09D62CBBA1DFBDD4691DAC96AD98B900000000000000000G",
"Invalid field \'marker\'."));

// this lambda function is used to create some fake marker using given
// taxon and sequence because we want to test some unassociated markers
// later
auto createFakeNFTMarker = [](AccountID const& issuer,
std::uint32_t taxon,
std::uint32_t tokenSeq,
std::uint16_t flags = 0,
std::uint16_t fee = 0) {
// the marker has the exact same format as an NFTokenID
return to_string(NFTokenMint::createNFTokenID(
flags, fee, issuer, nft::toTaxon(taxon), tokenSeq));
};

// test an unassociated marker which does not exist in the NFTokenIDs
BEAST_EXPECT(testInvalidMarker(
createFakeNFTMarker(bob.id(), 0x000000000, 0x00000000),
"Invalid field \'marker\'."));

// test an unassociated marker which exceeds the maximum value of the
// existing NFTokenID
BEAST_EXPECT(testInvalidMarker(
createFakeNFTMarker(bob.id(), 0xFFFFFFFF, 0xFFFFFFFF),
"Invalid field \'marker\'."));
}

void
Expand All @@ -1090,11 +1163,11 @@ class AccountObjects_test : public beast::unit_test::suite
testUnsteppedThenStepped();
testUnsteppedThenSteppedWithNFTs();
testObjectTypes();
testAccountNFTs();
testNFTsMarker();
}
};

BEAST_DEFINE_TESTSUITE(AccountObjects, rpc, ripple);
BEAST_DEFINE_TESTSUITE(AccountObjects, app, ripple);

} // namespace test
} // namespace ripple
1 change: 1 addition & 0 deletions src/xrpld/app/tx/detail/NFTokenMint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include <xrpld/app/tx/detail/NFTokenUtils.h>
#include <xrpld/app/tx/detail/Transactor.h>
#include <xrpl/protocol/nft.h>

namespace ripple {

Expand Down
28 changes: 22 additions & 6 deletions src/xrpld/rpc/handlers/AccountObjects.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ doAccountNFTs(RPC::JsonContext& context)
return *err;

uint256 marker;
bool const markerSet = params.isMember(jss::marker);

if (params.isMember(jss::marker))
if (markerSet)
{
auto const& m = params[jss::marker];
if (!m.isString())
Expand All @@ -100,6 +101,7 @@ doAccountNFTs(RPC::JsonContext& context)

// Continue iteration from the current page:
bool pastMarker = marker.isZero();
bool markerFound = false;
uint256 const maskedMarker = marker & nft::pageMask;
while (cp)
{
Expand All @@ -121,12 +123,23 @@ doAccountNFTs(RPC::JsonContext& context)
uint256 const nftokenID = o[sfNFTokenID];
uint256 const maskedNftokenID = nftokenID & nft::pageMask;

if (!pastMarker && maskedNftokenID < maskedMarker)
continue;
if (!pastMarker)
{
if (maskedNftokenID < maskedMarker)
continue;

if (!pastMarker && maskedNftokenID == maskedMarker &&
nftokenID <= marker)
continue;
if (maskedNftokenID == maskedMarker && nftokenID < marker)
continue;

if (nftokenID == marker)
{
markerFound = true;
continue;
}
}

if (markerSet && !markerFound)
return RPC::invalid_field_error(jss::marker);

pastMarker = true;

Expand Down Expand Up @@ -157,6 +170,9 @@ doAccountNFTs(RPC::JsonContext& context)
cp = nullptr;
}

if (markerSet && !markerFound)
return RPC::invalid_field_error(jss::marker);

result[jss::account] = toBase58(accountID);
context.loadType = Resource::feeMediumBurdenRPC;
return result;
Expand Down

0 comments on commit db238bc

Please sign in to comment.