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

fixDisallowIncomingV1 #4721

Merged
merged 10 commits into from
Oct 5, 2023
Merged

fixDisallowIncomingV1 #4721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
33c6445
fixDisallowIncomingV1
dangell7 Sep 21, 2023
86de183
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Sep 21, 2023
7df5bbc
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Sep 22, 2023
b9c18c9
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Sep 25, 2023
3b1b578
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Sep 26, 2023
9bc4f56
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Sep 28, 2023
4947453
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Oct 2, 2023
016dce8
add comment
dangell7 Oct 4, 2023
7864363
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Oct 4, 2023
cf88d62
Merge branch 'develop' into fixDisallowIncomingV1
dangell7 Oct 5, 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
15 changes: 14 additions & 1 deletion src/ripple/app/tx/impl/SetTrust.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,20 @@ SetTrust::preclaim(PreclaimContext const& ctx)
return tecNO_DST;

if (sleDst->getFlags() & lsfDisallowIncomingTrustline)
return tecNO_PERMISSION;
{
// The original implementation of featureDisallowIncoming was
// too restrictive. If
// o fixDisallowIncomingV1 is enabled and
// o The trust line already exists
// Then allow the TrustSet.
if (ctx.view.rules().enabled(fixDisallowIncomingV1) &&
dangell7 marked this conversation as resolved.
Show resolved Hide resolved
ctx.view.exists(keylet::line(id, uDstAccountID, currency)))
{
// pass
}
else
return tecNO_PERMISSION;
Comment on lines +149 to +155
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just invert the if and return inside it, instead of having an empty if and returning in the else?

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 actually have it like that on my local branch but with clang-format it makes it very "compact". I'm open to doing it like that if its preferred. It was just done like that for readability.

if (ctx.view.rules().enabled(fixDisallowIncomingV1) &&
        !ctx.view.exists(keylet::line(id, uDstAccountID, currency)))
        return tecNO_PERMISSION;

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general positive conditions should be preferred over negative conditions. They are quick and easy to scan through, it's very obvious to the reader what's happening. The more complicated the condition the more likely the next programmer will misunderstand it and insert a mistake.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I have no trouble reading the current formulation of the if. So I'm good with leaving it the way it is.

}
}

// If destination is AMM and the trustline doesn't exist then only
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ namespace detail {
// Feature.cpp. Because it's only used to reserve storage, and determine how
// large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
// the actual number of amendments. A LogicError on startup will verify this.
static constexpr std::size_t numFeatures = 62;
static constexpr std::size_t numFeatures = 63;

/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -349,6 +349,7 @@ extern uint256 const fixNFTokenRemint;
extern uint256 const fixReducedOffersV1;
extern uint256 const featureClawback;
extern uint256 const featureXChainBridge;
extern uint256 const fixDisallowIncomingV1;

} // namespace ripple

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,7 @@ REGISTER_FIX (fixReducedOffersV1, Supported::yes, VoteBehavior::De
REGISTER_FEATURE(Clawback, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::DefaultNo);
REGISTER_FIX(fixDisallowIncomingV1, Supported::yes, VoteBehavior::DefaultNo);

// The following amendments are obsolete, but must remain supported
// because they could potentially get enabled.
Expand Down
42 changes: 42 additions & 0 deletions src/test/app/SetTrust_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,48 @@ class SetTrust_test : public beast::unit_test::suite
BEAST_EXPECT(!(flags & lsfDisallowIncomingTrustline));
}

// fixDisallowIncomingV1
dangell7 marked this conversation as resolved.
Show resolved Hide resolved
{
for (bool const withFix : {true, false})
{
auto const amend = withFix
? features | disallowIncoming
: (features | disallowIncoming) - fixDisallowIncomingV1;

Env env{*this, amend};
auto const dist = Account("dist");
auto const gw = Account("gw");
auto const USD = gw["USD"];
auto const distUSD = dist["USD"];

env.fund(XRP(1000), gw, dist);
env.close();

env(fset(gw, asfRequireAuth));
env.close();

env(fset(dist, asfDisallowIncomingTrustline));
env.close();

env(trust(dist, USD(10000)));
env.close();

// withFix: can set trustline
// withOutFix: cannot set trustline
auto const trustResult =
withFix ? ter(tesSUCCESS) : ter(tecNO_PERMISSION);
env(trust(gw, distUSD(10000)),
txflags(tfSetfAuth),
trustResult);
env.close();

auto const txResult =
withFix ? ter(tesSUCCESS) : ter(tecPATH_DRY);
env(pay(gw, dist, USD(1000)), txResult);
env.close();
}
}

Env env{*this, features | disallowIncoming};

auto const gw = Account{"gateway"};
Expand Down