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.

Sign up for GitHub

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

Add allowClawback flag for account_info #4590

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Add allowClawback flag for account_info #4590

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
906dabe
allow clawback
shawnxie999 Jun 27, 2023
b04f17a
clang
shawnxie999 Jun 27, 2023
9b20f8c
Merge branch 'develop' into accountinfo_claw
shawnxie999 Jun 29, 2023
5d56b97
Merge branch 'develop' into accountinfo_claw
shawnxie999 Jul 4, 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
8 changes: 8 additions & 0 deletions src/ripple/rpc/handlers/AccountInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ doAccountInfo(RPC::JsonContext& context)
{"disallowIncomingPayChan", lsfDisallowIncomingPayChan},
{"disallowIncomingTrustline", lsfDisallowIncomingTrustline}}};

static constexpr std::pair<std::string_view, LedgerSpecificFlags>
allowClawbackFlag{"allowClawback", lsfAllowClawback};

auto const sleAccepted = ledger->read(keylet::account(accountID));
if (sleAccepted)
{
Expand Down Expand Up @@ -123,6 +126,11 @@ doAccountInfo(RPC::JsonContext& context)
for (auto const& lsf : disallowIncomingFlags)
acctFlags[lsf.first.data()] = sleAccepted->isFlag(lsf.second);
}

if (ledger->rules().enabled(featureClawback))
acctFlags[allowClawbackFlag.first.data()] =
sleAccepted->isFlag(allowClawbackFlag.second);

result[jss::account_flags] = std::move(acctFlags);

// Return SignerList(s) if that is requested.
Expand Down
43 changes: 35 additions & 8 deletions src/test/rpc/AccountInfo_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,13 +508,16 @@ class AccountInfo_test : public beast::unit_test::suite

Env env(*this, features);
Account const alice{"alice"};
env.fund(XRP(1000), alice);
Account const bob{"bob"};
env.fund(XRP(1000), alice, bob);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it required to fund bob's account in order to test the account flags?

Also, shouldn't the clawback feature be tested with a custom token (different from XRP) ?

Copy link
Collaborator Author

@shawnxie999 shawnxie999 Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Custom token is only needed for trustlines, we don’t do that here. This line is to just fund bob so that it becomes a valid account

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay. But the clawback feature is not applicable for XRP token asset right?
Isn't it better to perform this check on an account with custom tokens, instead of XRP tokens?

It appears that we are only checking if certain flags are set/unset, so I guess it doesn't matter.


auto getAccountFlag = [&env, &alice](std::string_view fName) {
auto getAccountFlag = [&env](
std::string_view fName,
Account const& account) {
auto const info = env.rpc(
"json",
"account_info",
R"({"account" : ")" + alice.human() + R"("})");
R"({"account" : ")" + account.human() + R"("})");

std::optional<bool> res;
if (info[jss::result][jss::status] == "success" &&
Expand Down Expand Up @@ -542,15 +545,15 @@ class AccountInfo_test : public beast::unit_test::suite
// as expected
env(fclear(alice, asf.second));
env.close();
auto const f1 = getAccountFlag(asf.first);
auto const f1 = getAccountFlag(asf.first, alice);
BEAST_EXPECT(f1.has_value());
BEAST_EXPECT(!f1.value());

// Set a flag and check that account_info returns results
// as expected
env(fset(alice, asf.second));
env.close();
auto const f2 = getAccountFlag(asf.first);
auto const f2 = getAccountFlag(asf.first, alice);
BEAST_EXPECT(f2.has_value());
BEAST_EXPECT(f2.value());
}
Expand All @@ -573,15 +576,15 @@ class AccountInfo_test : public beast::unit_test::suite
// as expected
env(fclear(alice, asf.second));
env.close();
auto const f1 = getAccountFlag(asf.first);
auto const f1 = getAccountFlag(asf.first, alice);
BEAST_EXPECT(f1.has_value());
BEAST_EXPECT(!f1.value());

// Set a flag and check that account_info returns results
// as expected
env(fset(alice, asf.second));
env.close();
auto const f2 = getAccountFlag(asf.first);
auto const f2 = getAccountFlag(asf.first, alice);
BEAST_EXPECT(f2.has_value());
BEAST_EXPECT(f2.value());
}
Expand All @@ -590,9 +593,31 @@ class AccountInfo_test : public beast::unit_test::suite
{
for (auto& asf : disallowIncomingFlags)
{
BEAST_EXPECT(!getAccountFlag(asf.first));
BEAST_EXPECT(!getAccountFlag(asf.first, alice));
}
}

static constexpr std::pair<std::string_view, std::uint32_t>
allowClawbackFlag{"allowClawback", asfAllowClawback};

if (features[featureClawback])
{
// must use bob's account because alice has noFreeze set
auto const f1 = getAccountFlag(allowClawbackFlag.first, bob);
BEAST_EXPECT(f1.has_value());
BEAST_EXPECT(!f1.value());

// Set allowClawback
env(fset(bob, allowClawbackFlag.second));
env.close();
auto const f2 = getAccountFlag(allowClawbackFlag.first, bob);
BEAST_EXPECT(f2.has_value());
BEAST_EXPECT(f2.value());
}
else
{
BEAST_EXPECT(!getAccountFlag(allowClawbackFlag.first, bob));
}
}

void
Expand All @@ -607,6 +632,8 @@ class AccountInfo_test : public beast::unit_test::suite
ripple::test::jtx::supported_amendments()};
testAccountFlags(allFeatures);
testAccountFlags(allFeatures - featureDisallowIncoming);
testAccountFlags(
allFeatures - featureDisallowIncoming - featureClawback);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we removing the featureClawback from set of all features? Shouldn't that be enabled for us to run the unit tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Amendments are included by default I think. So it has already been tested on lines 633 and 634

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh okay, got it.

}
};

Expand Down