fixDisallowIncomingV1 #4721

dangell7 · 2023-09-21T18:22:23Z

This commit introduces a new fix amendment (fixDisallowIncomingV1) which fixes an issue that occurs with authorized trustlines and the lsfDisallowIncomingTrustline flag.

Currently if a user creates an authorized trustline then sets the lsfDisallowIncomingTrustline flag on their account, the gateway/issuer cannot then approve the trustline.

To recreate the issue:

Issuer sets asfRequireAuth on their account.
User sets asfDisallowIncomingTrustline on their account.
User submits SetTrust tx to Issuer.
Issuer cannot then authorize the trustline.

Once the amendment is activated, the issuer can authorize the trustline after the User sets the asfDisallowIncomingTrustline flag because the trustline already exists.

mvadari · 2023-09-21T21:32:33Z

src/ripple/app/tx/impl/SetTrust.cpp

+            if (ctx.view.rules().enabled(fixDisallowIncomingV1) &&
+                ctx.view.exists(keylet::line(id, uDstAccountID, currency)))
+            {
+                // pass
+            }
+            else
+                return tecNO_PERMISSION;


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

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;

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.

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

scottschurr · 2023-09-28T17:24:00Z

I just looked over this pull request. The proposed approach looks like it would work and solve the identified problem.

However, I'm not yet convinced that the identified problem justifies an amendment. Let me walk through why.

If asfDisallowIncomingTrustline were a sticky flag (that couldn't be cleared), then I'd say you have a great justification. But the flag is not sticky. asfDisallowIncomingTrustline can be set and cleared as often as an account wants. So if an account gets into the situation solved by this fix, they can also solve the problem by

Clearing their asfDisallowIncomingTrustline flag briefly,
Letting the tfSetfAuth get set on the trust line, and then
Re-enabling their asfDisallowIncomingTrustline flag.

This unit test fragment demonstrates the approach I'm suggesting.

        {
            Env env{*this, (features | disallowIncoming) - fixDisallowIncomingV1};
            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();

            // Can't set trust line because dist has set the
            // asfDisallowIncomingTrustline flag.
            env(trust(gw, distUSD(10000)),
                txflags(tfSetfAuth),
                ter(tecNO_PERMISSION));
            env.close();

            // But dist can clear the asfDisallowIncomingTrustline flag.  Then
            // the trust line can have `tfSetfAuth` set.
            env(fclear(dist, asfDisallowIncomingTrustline));
            env.close();

            env(trust(gw, distUSD(10000)), txflags(tfSetfAuth));
            env.close();

            // Now dist can re-enable asfDisallowIncomingTrustline.
            env(fclear(dist, asfDisallowIncomingTrustline));
            env.close();

            env(pay(gw, dist, USD(1000)));
            env.close();
        }

This approach has the advantage that it works today (without an amendment) and it's pretty easy to explain.

Is it elegant? No. But it solves the problem for what I suspect is a pretty small user population.

I'm also open to being wrong on this. Convince me that this proposed amendment has a good justification.

Thanks for reading.

dangell7 · 2023-09-28T18:38:47Z

I just looked over this pull request. The proposed approach looks like it would work and solve the identified problem.

However, I'm not yet convinced that the identified problem justifies an amendment. Let me walk through why.

If asfDisallowIncomingTrustline were a sticky flag (that couldn't be cleared), then I'd say you have a great justification. But the flag is not sticky. asfDisallowIncomingTrustline can be set and cleared as often as an account wants. So if an account gets into the situation solved by this fix, they can also solve the problem by

Clearing their asfDisallowIncomingTrustline flag briefly,

Letting the tfSetfAuth get set on the trust line, and then

Re-enabling their asfDisallowIncomingTrustline flag.

This unit test fragment demonstrates the approach I'm suggesting.
        {
            Env env{*this, (features | disallowIncoming) - fixDisallowIncomingV1};
            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();

            // Can't set trust line because dist has set the
            // asfDisallowIncomingTrustline flag.
            env(trust(gw, distUSD(10000)),
                txflags(tfSetfAuth),
                ter(tecNO_PERMISSION));
            env.close();

            // But dist can clear the asfDisallowIncomingTrustline flag.  Then
            // the trust line can have `tfSetfAuth` set.
            env(fclear(dist, asfDisallowIncomingTrustline));
            env.close();

            env(trust(gw, distUSD(10000)), txflags(tfSetfAuth));
            env.close();

            // Now dist can re-enable asfDisallowIncomingTrustline.
            env(fclear(dist, asfDisallowIncomingTrustline));
            env.close();

            env(pay(gw, dist, USD(1000)));
            env.close();
        }
This approach has the advantage that it works today (without an amendment) and it's pretty easy to explain.

Is it elegant? No. But it solves the problem for what I suspect is a pretty small user population.

I'm also open to being wrong on this. Convince me that this proposed amendment has a good justification.

Thanks for reading.

I would argue a few things.

This means that the issuer needs to communicate to me, for me to clear my flag then reset it.
They/I would need to do this every time, for every issuer that has an authorized trustline.
I already gave permission/trust to the issuer through adding their trustline therefore this process is redundant.

scottschurr · 2023-10-03T15:57:30Z

@dangell7 wrote:

I would argue a few things.

This means that the issuer needs to communicate to me, for me to clear my flag then reset it.

They/I would need to do this every time, for every issuer that has an authorized trustline.

I already gave permission/trust to the issuer through adding their trustline therefore this process is redundant.

Thanks for bringing those up. I think all of those are legitimate points. However I'm not completely convinced they are important enough to merit an amendment.

On the other hand I recognize that I'm not a very good representative of ledger usability concerns. So my hope is that someone with more usability focus will weigh in. Possibly @mvadari or @ximinez?

mvadari · 2023-10-03T16:36:07Z

@dangell7 wrote:

I would argue a few things.

This means that the issuer needs to communicate to me, for me to clear my flag then reset it.

They/I would need to do this every time, for every issuer that has an authorized trustline.

I already gave permission/trust to the issuer through adding their trustline therefore this process is redundant.

Thanks for bringing those up. I think all of those are legitimate points. However I'm not completely convinced they are important enough to merit an amendment.

On the other hand I recognize that I'm not a very good representative of ledger usability concerns. So my hope is that someone with more usability focus will weigh in. Possibly @mvadari or @ximinez?

While I agree that the flow @scottschurr described would be an easier solution to the problem (easier in that it doesn't require enabling an amendment), I think this amendment fixes the behavior to be what it should be. IMO this amendment should still be approved, but in the meanwhile we can use other mechanisms to deal with the problem (and the explanations will be a bit simpler when the fix is enabled). I'm of the opinion that if a fix amendment isn't going to change the user API (which would be a pain to deal with in tooling), then there's no reason why it shouldn't be in the codebase.

scottschurr

👍 Nice job with this pull request. The implementation is straightforward and easy to read. The unit test demonstrates the intention and utility of the change.

I left one note about a comment that might be changed. But I leave that your discretion.

scottschurr · 2023-10-03T23:28:19Z

src/ripple/app/tx/impl/SetTrust.cpp

+            if (ctx.view.rules().enabled(fixDisallowIncomingV1) &&
+                ctx.view.exists(keylet::line(id, uDstAccountID, currency)))
+            {
+                // pass
+            }
+            else
+                return tecNO_PERMISSION;


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

src/test/app/SetTrust_test.cpp

src/ripple/app/tx/impl/SetTrust.cpp

intelliot · 2023-10-05T00:08:02Z

This PR has sufficient approvals. @dangell7 , please:

Bring this branch up-to-date with develop
Confirm that, from your perspective, this PR is ready to merge.

Thanks!

dangell7 · 2023-10-05T07:08:05Z

LGTM

…F#4721) Context: The `DisallowIncoming` amendment provides an option to block incoming trust lines from reaching your account. The asfDisallowIncomingTrustline AccountSet Flag, when enabled, prevents any incoming trust line from being created. However, it was too restrictive: it would block an issuer from authorizing a trust line, even if the trust line already exists. Consider: 1. Issuer sets asfRequireAuth on their account. 2. User sets asfDisallowIncomingTrustline on their account. 3. User submits tx to SetTrust to Issuer. At this point, without `fixDisallowIncomingV1` active, the issuer would not be able to authorize the trust line. The `fixDisallowIncomingV1` amendment, once activated, allows an issuer to authorize a trust line even after the user sets the asfDisallowIncomingTrustline flag, as long as the trust line already exists.

dangell7 added 2 commits September 21, 2023 20:17

fixDisallowIncomingV1

33c6445

Merge branch 'develop' into fixDisallowIncomingV1

86de183

mvadari reviewed Sep 21, 2023

View reviewed changes

dangell7 added 3 commits September 22, 2023 22:38

Merge branch 'develop' into fixDisallowIncomingV1

7df5bbc

Merge branch 'develop' into fixDisallowIncomingV1

b9c18c9

Merge branch 'develop' into fixDisallowIncomingV1

3b1b578

Merge branch 'develop' into fixDisallowIncomingV1

9bc4f56

intelliot assigned scottschurr Oct 2, 2023

intelliot requested review from ximinez and scottschurr October 2, 2023 14:43

Merge branch 'develop' into fixDisallowIncomingV1

4947453

scottschurr approved these changes Oct 4, 2023

View reviewed changes

intelliot added Tx Change Amendment labels Oct 4, 2023

intelliot requested review from RichardAH and mvadari October 4, 2023 04:39

intelliot unassigned scottschurr Oct 4, 2023

dangell7 added 2 commits October 4, 2023 09:16

add comment

016dce8

Merge branch 'develop' into fixDisallowIncomingV1

7864363

mvadari approved these changes Oct 4, 2023

View reviewed changes

Merge branch 'develop' into fixDisallowIncomingV1

cf88d62

intelliot merged commit 6ba9450 into XRPLF:develop Oct 5, 2023
15 checks passed

intelliot added this to the 2.0 milestone Oct 5, 2023

intelliot mentioned this pull request Oct 17, 2023

Proposed 2.0.0-b3 #4772

Merged

1 task

intelliot added the Testable label Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixDisallowIncomingV1 #4721

fixDisallowIncomingV1 #4721

dangell7 commented Sep 21, 2023

mvadari Sep 21, 2023

dangell7 Sep 22, 2023

RichardAH Sep 24, 2023

scottschurr Oct 3, 2023

scottschurr commented Sep 28, 2023

dangell7 commented Sep 28, 2023

scottschurr commented Oct 3, 2023

mvadari commented Oct 3, 2023

scottschurr left a comment

scottschurr Oct 3, 2023

intelliot commented Oct 5, 2023

dangell7 commented Oct 5, 2023

fixDisallowIncomingV1 #4721

fixDisallowIncomingV1 #4721

Conversation

dangell7 commented Sep 21, 2023

mvadari Sep 21, 2023

Choose a reason for hiding this comment

dangell7 Sep 22, 2023

Choose a reason for hiding this comment

RichardAH Sep 24, 2023

Choose a reason for hiding this comment

scottschurr Oct 3, 2023

Choose a reason for hiding this comment

scottschurr commented Sep 28, 2023

dangell7 commented Sep 28, 2023

scottschurr commented Oct 3, 2023

mvadari commented Oct 3, 2023

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Oct 3, 2023

Choose a reason for hiding this comment

intelliot commented Oct 5, 2023

dangell7 commented Oct 5, 2023