Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC commands understand markers derived from all ledger object types #4361

Merged
merged 18 commits into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
31b9ddc
RPC commands understand markers derived from all ledger object types:
ximinez Dec 1, 2022
b16593f
[FOLD] Review feedback from @scottschurr:
ximinez Dec 7, 2022
11f896a
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Jan 5, 2023
fc02cb8
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Jan 6, 2023
9f9cda7
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Jan 12, 2023
b88e1f7
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 3, 2023
401c85d
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 6, 2023
be7e917
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 8, 2023
789788c
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 9, 2023
6d7628b
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 15, 2023
c0717b9
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 17, 2023
5764c2a
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 23, 2023
4f23caf
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 23, 2023
0dcc806
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Feb 28, 2023
82c6e9d
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Mar 3, 2023
c042b68
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Mar 14, 2023
ddd3d98
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Mar 14, 2023
d6883e6
Merge remote-tracking branch 'upstream/develop' into badmarker
ximinez Mar 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ripple/rpc/handlers/AccountChannels.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ doAccountChannels(RPC::JsonContext& context)
if (!sle)
return rpcError(rpcINVALID_PARAMS);

if (!RPC::isOwnedByAccount(*ledger, sle, accountID))
if (!RPC::isRelatedToAccount(*ledger, sle, accountID))
return rpcError(rpcINVALID_PARAMS);
}

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/handlers/AccountLines.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ doAccountLines(RPC::JsonContext& context)
if (!sle)
return rpcError(rpcINVALID_PARAMS);

if (!RPC::isOwnedByAccount(*ledger, sle, accountID))
if (!RPC::isRelatedToAccount(*ledger, sle, accountID))
return rpcError(rpcINVALID_PARAMS);
}

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/handlers/AccountOffers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ doAccountOffers(RPC::JsonContext& context)
if (!sle)
return rpcError(rpcINVALID_PARAMS);

if (!RPC::isOwnedByAccount(*ledger, sle, accountID))
if (!RPC::isRelatedToAccount(*ledger, sle, accountID))
return rpcError(rpcINVALID_PARAMS);
}

Expand Down
19 changes: 17 additions & 2 deletions src/ripple/rpc/impl/RPCHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <ripple/app/misc/Transaction.h>
#include <ripple/app/paths/TrustLine.h>
#include <ripple/app/rdb/RelationalDatabase.h>
#include <ripple/app/tx/impl/details/NFTokenUtils.h>
#include <ripple/ledger/View.h>
#include <ripple/net/RPCErr.h>
#include <ripple/protocol/AccountID.h>
Expand Down Expand Up @@ -109,7 +110,7 @@ getStartHint(std::shared_ptr<SLE const> const& sle, AccountID const& accountID)
}

bool
isOwnedByAccount(
isRelatedToAccount(
ReadView const& ledger,
std::shared_ptr<SLE const> const& sle,
AccountID const& accountID)
Expand All @@ -121,13 +122,27 @@ isOwnedByAccount(
}
else if (sle->isFieldPresent(sfAccount))
{
return sle->getAccountID(sfAccount) == accountID;
// 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) &&
ximinez marked this conversation as resolved.
Show resolved Hide resolved
sle->getAccountID(sfDestination) == accountID);
}
else if (sle->getType() == ltSIGNER_LIST)
{
Keylet const accountSignerList = keylet::signers(accountID);
return sle->key() == accountSignerList.key;
}
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;
}

return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/impl/RPCHelpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ getStartHint(std::shared_ptr<SLE const> const& sle, AccountID const& accountID);
* @param account - The account being tested for SLE ownership.
*/
bool
isOwnedByAccount(
isRelatedToAccount(
ReadView const& ledger,
std::shared_ptr<SLE const> const& sle,
AccountID const& accountID);
Expand Down
238 changes: 236 additions & 2 deletions src/test/rpc/AccountLinesRPC_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite
void
testAccountLines()
{
testcase("acccount_lines");
testcase("account_lines");
ximinez marked this conversation as resolved.
Show resolved Hide resolved

using namespace test::jtx;
Env env(*this);
Expand Down Expand Up @@ -524,11 +524,244 @@ class AccountLinesRPC_test : public beast::unit_test::suite
RPC::make_error(rpcINVALID_PARAMS)[jss::error_message]);
}

void
testAccountLinesWalkMarkers()
{
testcase("Marker can point to any appropriate ledger entry type");
using namespace test::jtx;
using namespace std::chrono_literals;
Env env(*this);

// The goal of this test is observe account_lines RPC calls return an
// error message when the SLE pointed to by the marker is not owned by
// the Account being traversed.
//
// To start, we'll create an environment with some trust lines, offers
// and a signers list.
Account const alice{"alice"};
Account const becky{"becky"};
Account const gw1{"gw1"};
env.fund(XRP(10000), alice, becky, gw1);
env.close();

// A couple of helper lambdas
auto escrow = [&env](
Account const& account,
Account const& to,
STAmount const& amount) {
Json::Value jv;
jv[jss::TransactionType] = jss::EscrowCreate;
jv[jss::Flags] = tfUniversal;
jv[jss::Account] = account.human();
jv[jss::Destination] = to.human();
jv[jss::Amount] = amount.getJson(JsonOptions::none);
NetClock::time_point finish = env.now() + 1s;
jv[sfFinishAfter.jsonName] = finish.time_since_epoch().count();
return jv;
};
Comment on lines +548 to +561
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm noticing that there are functions or manual code to create escrows scattered around the test subdirectory. I see them in...

  • Escrow_test.cpp,
  • AccountDelete_test.cpp,
  • LedgerRPC_test.cpp,
  • AccountObjects_test.cpp,
  • AccountTx_test.cpp, and
  • LedgerData_test.cpp

It's probably time we bit the bullet and wrote some common code to build escrows in the test environment. But I don't need to burden this pull request with that change. Just an observation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I actually stole my code from Escrow_test.cpp, I think.

Copy link
Collaborator

@dangell7 dangell7 Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can attempt to apply the solve in this PR if you let me know what you want.

#4396

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dangell7, thanks for the offer. I can't give you a precise definition of what I'm looking for, but here's a hand wave.

First, note that there are several different unit test files that locally define functions or lambdas which

  1. Create escrows,
  2. Finish escrows, or
  3. Cancel escrows.

Here's where those functions are defined (locally) in Escrow_test.cpp: https:/XRPLF/rippled/blob/develop/src/test/app/Escrow_test.cpp#L170-L190

Since this functions are only defined locally, they are not available to all of the test files that need to do these escrow operations. So the functions have been copied, pasted, and mutated into different test files. The escrow lambda, on line 548 here in this file, is one such example.

However, if we pulled those functions out of Escrow_test.cpp and put them somewhere they could be used by all the tests, then we can remove a lot of code duplication.

An example you could consider following is the src/test/jtx/check.h and src/test/jtx/impl/check.cpp files. Those files create a check namespace and put all of the check-related functions in there. So in tests the functions are invoked like check::create(...) or check::cash(...). That seems like a pretty good approach for less common facilities like checks and escrows.

But once you've made those functions in the escrow namespace the work has only started. Now you need to go through all of the tests that use escrows and remove all of those local functions and lambdas. Replace calls to those local functions will calls to the new functions you created in the escrow namespace. This may get tricky, because the escrow-related functions that have been copied around may have mutated. So the interfaces of the copied functions may not be uniform across the code base.

Once you've...

  1. Created the new globally accessible test functions,
  2. Removed all of the local definitions,
  3. Replaced calls to those local definitions with calls to the new global functions, and
  4. The unit tests all pass.

Then you're done.

Does that make sense? It may be more work than you want to take on in your pull request. It may make more sense as a separate pull request.


auto payChan = [](Account const& account,
Account const& to,
STAmount const& amount,
NetClock::duration const& settleDelay,
PublicKey const& pk) {
Json::Value jv;
jv[jss::TransactionType] = jss::PaymentChannelCreate;
jv[jss::Flags] = tfUniversal;
jv[jss::Account] = account.human();
jv[jss::Destination] = to.human();
jv[jss::Amount] = amount.getJson(JsonOptions::none);
jv["SettleDelay"] = settleDelay.count();
jv["PublicKey"] = strHex(pk.slice());
return jv;
};
Comment on lines +563 to +577
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note for escrows, above, applies to payChans as well. They could both be addressed in the same pull request, but not necessarily this pull request.

Copy link
Collaborator

@dangell7 dangell7 Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can attempt to apply the solve in this PR if you let me know what you want.

#4396

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dangell7. Thanks for this offer also. The notes here https:/XRPLF/rippled/pull/4361/files#r1109064286 describe what would be done for escrows. An almost identical process could be followed for PayChan. You can think about whether that's a task you would like to take on.


// Test all available object types. Not all of these objects will be
// included in the search, nor found by `account_objects`. If that ever
// changes for any reason, this test will help catch that.
//
// SignerList, for alice
Account const bogie{"bogie"};
env(signers(alice, 2, {{bogie, 3}}));
env.close();

// SignerList, includes alice
env(signers(becky, 2, {{alice, 3}}));
env.close();

// Trust lines
auto const EUR = gw1["EUR"];
env(trust(alice, EUR(200)));
env(trust(becky, EUR(200)));
env.close();

// Escrow, in each direction
env(escrow(alice, becky, XRP(1000)));
env(escrow(becky, alice, XRP(1000)));

// Pay channels, in each direction
env(payChan(alice, becky, XRP(1000), 100s, alice.pk()));
env(payChan(becky, alice, XRP(1000), 100s, becky.pk()));

// Mint NFTs, for each account
uint256 const aliceNFtokenID =
token::getNextID(env, alice, 0, tfTransferable);
env(token::mint(alice, 0), txflags(tfTransferable));

uint256 const beckyNFtokenID =
token::getNextID(env, becky, 0, tfTransferable);
env(token::mint(becky, 0), txflags(tfTransferable));

// NFT Offers, for each other's NFTs
env(token::createOffer(alice, beckyNFtokenID, drops(1)),
token::owner(becky));
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)));

// Deposit preauth, in each direction
env(deposit::auth(alice, becky));
env(deposit::auth(becky, alice));

// Offers, one where alice is the owner, and one where alice is the
// issuer
auto const USDalice = alice["USD"];
env(offer(alice, EUR(10), XRP(100)));
env(offer(becky, USDalice(10), XRP(100)));

// Tickets
env(ticket::create(alice, 2));

// Add another trustline for good measure
auto const BTCbecky = becky["BTC"];
env(trust(alice, BTCbecky(200)));

env.close();

{
// 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 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) {
return aliceLines[jss::result].isMember(jss::marker);
};
auto marker = [](auto const& aliceLines) {
return aliceLines[jss::result][jss::marker].asString();
};
auto checkLines = [](auto const& aliceLines) {
return aliceLines.isMember(jss::result) &&
!aliceLines[jss::result].isMember(jss::error_message) &&
aliceLines[jss::result].isMember(jss::lines) &&
aliceLines[jss::result][jss::lines].isArray() &&
aliceLines[jss::result][jss::lines].size() <= 1;
};

BEAST_EXPECT(hasMarker(aliceLines));
BEAST_EXPECT(checkLines(aliceLines));
BEAST_EXPECT(aliceLines[jss::result][jss::lines].size() == 0);

int iterations = 1;

while (hasMarker(aliceLines))
{
// Iterate through the markers
aliceLines = getNextLine(env, alice, marker(aliceLines));
BEAST_EXPECT(checkLines(aliceLines));
foundLines += aliceLines[jss::result][jss::lines].size();
++iterations;
}
BEAST_EXPECT(expectedLines == foundLines);

Json::Value const aliceObjects = env.rpc(
"json",
"account_objects",
R"({"account": ")" + alice.human() +
R"(", )"
R"("limit": 200})");
BEAST_EXPECT(aliceObjects.isMember(jss::result));
BEAST_EXPECT(
!aliceObjects[jss::result].isMember(jss::error_message));
BEAST_EXPECT(
aliceObjects[jss::result].isMember(jss::account_objects));
BEAST_EXPECT(
aliceObjects[jss::result][jss::account_objects].isArray());
// account_objects does not currently return NFTPages. If
// that ever changes, without also changing account_lines,
// this test will need to be updated.
BEAST_EXPECT(
aliceObjects[jss::result][jss::account_objects].size() ==
iterations);
// If ledger object association ever changes, for whatever
// reason, this test will need to be updated.
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);
}
}

// test API V2
void
testAccountLines2()
{
testcase("V2: acccount_lines");
testcase("V2: account_lines");

using namespace test::jtx;
Env env(*this);
Expand Down Expand Up @@ -1234,6 +1467,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite
testAccountLines();
testAccountLinesMarker();
testAccountLineDelete();
testAccountLinesWalkMarkers();
testAccountLines2();
testAccountLineDelete2();
}
Expand Down