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 <scott@ripple.com>
  • Loading branch information
@yinyiqian1
yinyiqian1 authored Jul 2, 2024
1 parent 9fec615 commit e1534a3
Show file tree
Hide file tree
Showing 3 changed files with 148 additions and 6 deletions.
125 changes: 125 additions & 0 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 @@ -1032,13 +1034,136 @@ class AccountObjects_test : public beast::unit_test::suite
BEAST_EXPECT(acct_objs_is_size(acct_objs(gw, jss::hashes), 0));
}

void
testNFTsMarker()
{
// 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);

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

static constexpr unsigned nftsSize = 10;
for (unsigned i = 0; i < nftsSize; i++)
{
env(token::mint(bob, 0));
}

env.close();

// 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
run() override
{
testErrors();
testUnsteppedThenStepped();
testUnsteppedThenSteppedWithNFTs();
testObjectTypes();
testNFTsMarker();
}
};

Expand Down
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 @@ -75,8 +75,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 @@ -98,6 +99,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 @@ -119,12 +121,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 @@ -155,6 +168,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 e1534a3

Please sign in to comment.