CheckCashMakesTrustLine amendment #3823

scottschurr · 2021-04-15T01:33:02Z

High Level Overview of Change

With this amendment, the CheckCash transaction creates a trust line if needed. The change is modeled after Offer crossing. And, similar to Offer crossing, cashing a Check allows an account to exceed its trust line limit.

Context of Change

It has been determined that the user experience of needing to create a trust line prior to cashing a Check was both inconvenient and not necessary. Similar to an Offer, anyone who cashes a Check clearly wants to accept the funds involved. Automatically creating the trust line, if necessary, would be convenient and has precedent with Offer crossing.

Therefore this amendment introduces two changes to the behavior of cashing a Check:

If the account cashing the Check does not already have a trust line for the currency being received, then that trust line is created if possible. It usually is possible.

Reasons it might not be possible to create the trust line include:
- The account cashing the check does not have sufficient XRP to meet the reserve required to create the trust line.
- Rippling would be required to transfer the funds and the issuing account has not set the lsfDefaultRipple flag.
- The account issuing the currency has set the lsfGlobalFreeze flag.
- The account issuing the currency has set the lsfRequireAuth flag, so an automatically created trust line would not be authorized.
The currency being brought into the account by the Check is allowed to exceed any limit specified the in trust line. There are three reasons for this:
- If the account is explicitly cashing the Check then they must want the currency in question.
- Offers have the same behavior. It makes sense to limit the number of unique behaviors on the ledger.
- An automatically created trust line has limits of zero. If we didn't have this rule then the automatically created trust line would be useless.

If cashing the Check fails for any reason, then the trust line is not added to the account.

Type of Change

Breaking change (a feature that would cause existing functionality to not work as expected)
Tests (added unit tests)
Documentation updates will be required

If this change is merged then documentation will need to be updated to cover the behavior changes. This change will also need to be mentioned in the release notes.

Test Plan

Unit tests were added to verify that prior to the introduction of the amendment cashing a Check without a pre-existing trust line would fail. After the introduction of the amendment cashing a Check without a pre-existing trust line succeeds by adding the necessary trust line to the ledger.

A number of different scenarios were tested including:

Cashing a Check that was created by the issuer of the currency in question.
Cashing a Check written by an account other than the issuer.
A variety of AccountRoot flags were tested which either allowed or prevented the automatic trust line creation.
Trust lines automatically created by Offer crossing were compared to trust lines automatically created by Check cashing and found to be very similar (as intended).

Requests of Reviewers

Please consider situations I might have forgotten which might affect creation of a trust line. I tested the account root flags I thought would matter. Did I miss important account root flags? Are there conditions other than account root flags that need to be tested? Thanks for the help.

Future Tasks

Once a trust line is added to the ledger, it can be challenging for a ledger user to remove the trust line. It may be useful to explore ways for non-issuer accounts to more easily remove trust lines.

intelliot · 2021-04-23T02:30:53Z

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

                    // Set the delivered_amount metadata.
                    ctx_.deliver(result.actualAmountOut);
            }
+            // Set the delivered amount metadata in all cases, not just


Prior to checkCashMakesTrustLine, why was the delivered amount metadata set only if DeliverMin was used?

Hi @intelliot, I believe that was a bug. As part of this pull request I added some unit tests. I expected that in all cases the result of a successful CheckCash transactions would include a delivered_amount field. I found that delivered_amount was not always set. I surmised delivered_amount was not set because DeliveredAmount was not always set due to the conditional above.

When I changed the code so setting DeliveredAmount was only conditional on the amendment then the unit tests returned a delivery_amount field in all of the tested cases.

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

src/test/app/Check_test.cpp

src/test/jtx/impl/Env.cpp

scottschurr

Thanks for the great review @ximinez! I've pushed a commit that I believe addresses your comments. Please let me know if I missed anything.

I've also rebased on top of #3840 since it has passed and provides functionality that I'm using. I anticipate that #3840 will be merged before this pull request is.

Any future reviewers will want to skip the commits below the one named Introduce CheckCashMakesTrustLine amendment.

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

src/test/app/Check_test.cpp

scottschurr · 2021-05-28T20:24:41Z

@ximinez, thanks for the suggestion regarding the macro. I followed your lead but tweaked it a bit. At your convenience take a look and see what you think.

With this amendment, the CheckCash transaction creates a TrustLine if needed. The change is modeled after offer crossing. And, similar to offer crossing, cashing a check allows an account to exceed its trust line limit.

scottschurr · 2021-06-04T22:25:16Z

Rebased to 1.8.0-b3.

ximinez

Sorry for the delay on this followup. The latest changes look good!

ximinez · 2021-06-08T19:13:31Z

src/test/app/Check_test.cpp

+                suite.expect(
+                    ownerCount(env, acct) == owners,
+                    "Owner count mismatch",
+                    __FILE__,
+                    line);


scottschurr · 2021-06-09T20:18:37Z

@nbougalis is really busy right now, so I'm swapping in @thejohnfreeman as a technical reviewer. Thanks for the help, @thejohnfreeman!

thejohnfreeman

Can we add a test that a second check for the same currency can be cashed after one that created a trust line (i.e. that the limits are ignored even when a trust line already existed)?

thejohnfreeman · 2021-06-10T16:01:14Z

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

-                JLOG(ctx.j.warn())
-                    << "Cannot cash check for IOU without trustline.";
-                return tecNO_LINE;
+                if (!ctx.view.rules().enabled(featureCheckCashMakesTrustLine))


One way to do the changes from line 200 (in the original code) to line 225 that requires fewer actual code changes is to just add && !ctx.view.rules().enabled(featureCheckCashMakesTrustLine) to the condition on line 200, and insert this condition on line 218:

if (!sleTrustLine) { return tecNO_AUTH; }

I believe you, but I'm not really clear on the edit you'd like to see. Can you spell out the proposed change further? Thanks.

Here it is as a commit: thejohnfreeman@d57c0e8

thejohnfreeman · 2021-06-10T16:18:38Z

src/test/app/Check_test.cpp

+        // All remaining tests require featureCheckCashMakesTrustLine.
+        if (!features[featureCheckCashMakesTrustLine])
+            return;


With CheckCashMakesTrustLine disabled, this function is only the test above this return, which duplicates a test elsewhere. I personally find it easier to understand if this function is only run with CheckCashMakesTrustLine enabled, without lines 1943 - 1974 (the sanity check and early return), but maybe with an assertion that the feature is enabled.

scottschurr · 2021-06-11T20:04:19Z

Thanks for the great review @thejohnfreeman. I've pushed a couple of commits that I believe address your comments. Please let me know if there's anything else you'd like addressed.

ximinez

Still looks good. I like the changes.

thejohnfreeman · 2021-06-17T21:45:42Z

Looks great! Thank you!

scottschurr · 2021-06-17T22:52:51Z

@mDuo13 and @intelliot, does this change look reasonable to you? I want to make sure before I mark it passed. Thanks.

mDuo13 · 2021-06-18T10:27:58Z

Tentatively, looks good.

I certainly appreciate the idea, and the semantics as laid out in the description sound perfect to me. I haven't tested out the changes or looked through the code to confirm that it works as described or that there aren't any relevant edge cases I haven't thought of.

To be clear, when cashing a Check for a "flexible" amount, is the amount it delivers is no longer bounded by the Check recipient's trust limits (even if there's an existing line and trust limits are set to nonzero values)? That's sensible to me, though it may be surprising in at least a couple cases, so it's something to make sure the docs clarify. I guess in that case, the only things that may limit the redemption to less than the Check's full amount are:

(a) the amount the check sender holds, if they aren't the issuer
(b) the transfer fee, if the check sender is not the issuer

Right?

scottschurr · 2021-06-21T18:55:58Z

@mDuo13, that sounds correct.

scottschurr added the Amendment label Apr 15, 2021

scottschurr requested review from intelliot, nbougalis, ximinez and mDuo13 April 15, 2021 01:33

scottschurr assigned intelliot, nbougalis, ximinez and mDuo13 Apr 15, 2021

mDuo13 mentioned this pull request Apr 22, 2021

Document CheckCashMakesTrustLine amendment XRPLF/xrpl-dev-portal#1048

Closed

intelliot reviewed Apr 23, 2021

View reviewed changes

ximinez requested changes May 14, 2021

View reviewed changes

scottschurr force-pushed the check-trust branch from 74732c3 to d897bca Compare May 17, 2021 21:24

scottschurr commented May 17, 2021

View reviewed changes

src/ripple/app/tx/impl/CashCheck.cpp Outdated Show resolved Hide resolved

src/test/app/Check_test.cpp Outdated Show resolved Hide resolved

src/test/app/Check_test.cpp Outdated Show resolved Hide resolved

src/test/app/Check_test.cpp Show resolved Hide resolved

ximinez requested changes May 27, 2021

View reviewed changes

src/test/app/Check_test.cpp Outdated Show resolved Hide resolved

src/test/app/Check_test.cpp Outdated Show resolved Hide resolved

src/test/app/Check_test.cpp Show resolved Hide resolved

scottschurr added 3 commits June 4, 2021 14:29

Introduce CheckCashMakesTrustLine amendment:

40ebcca

With this amendment, the CheckCash transaction creates a TrustLine if needed. The change is modeled after offer crossing. And, similar to offer crossing, cashing a check allows an account to exceed its trust line limit.

[FOLD] Address review comments

db69a8e

[FOLD] Replace macro with struct

f25ee5e

scottschurr force-pushed the check-trust branch from c444f98 to f25ee5e Compare June 4, 2021 22:23

ximinez approved these changes Jun 8, 2021

View reviewed changes

scottschurr requested review from thejohnfreeman and removed request for nbougalis June 9, 2021 20:17

scottschurr assigned thejohnfreeman and unassigned nbougalis Jun 9, 2021

thejohnfreeman reviewed Jun 10, 2021

View reviewed changes

thejohnfreeman and others added 2 commits June 11, 2021 11:16

[fold] Same behavior with fewer edits

dea55ab

[FOLD] Address review comments regarding unit tests

69a4b43

scottschurr force-pushed the check-trust branch from 3d6c2ca to 69a4b43 Compare June 11, 2021 20:01

ximinez approved these changes Jun 14, 2021

View reviewed changes

thejohnfreeman approved these changes Jun 17, 2021

View reviewed changes

SirJosh1987 approved these changes Jul 9, 2021

View reviewed changes

SirJosh1987 approved these changes Jul 10, 2021

View reviewed changes

manojsdoshi added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 20, 2021

manojsdoshi mentioned this pull request Jul 27, 2021

Proposed 1.8.0-b4 #3889

Merged

manojsdoshi closed this in #3889 Jul 28, 2021

scottschurr deleted the check-trust branch November 3, 2021 16:29

mDuo13 mentioned this pull request Nov 10, 2021

XRPL server v1.8.0 docs XRPLF/xrpl-dev-portal#1233

Merged

12 tasks

This was referenced Feb 11, 2023

Cashing a check should automatically create a trustline #3790

Closed

Currency conversion should automatically create a trustline #4415

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CheckCashMakesTrustLine amendment #3823

CheckCashMakesTrustLine amendment #3823

scottschurr commented Apr 15, 2021

intelliot Apr 23, 2021

scottschurr Apr 26, 2021

scottschurr left a comment

scottschurr commented May 28, 2021

scottschurr commented Jun 4, 2021

ximinez left a comment

ximinez Jun 8, 2021

scottschurr commented Jun 9, 2021

thejohnfreeman left a comment

thejohnfreeman Jun 10, 2021

scottschurr Jun 10, 2021

thejohnfreeman Jun 11, 2021

thejohnfreeman Jun 10, 2021

scottschurr commented Jun 11, 2021

ximinez left a comment

thejohnfreeman commented Jun 17, 2021

scottschurr commented Jun 17, 2021

mDuo13 commented Jun 18, 2021 •

edited

Loading

scottschurr commented Jun 21, 2021

CheckCashMakesTrustLine amendment #3823

CheckCashMakesTrustLine amendment #3823

Conversation

scottschurr commented Apr 15, 2021

High Level Overview of Change

Context of Change

Type of Change

Test Plan

Requests of Reviewers

Future Tasks

intelliot Apr 23, 2021

Choose a reason for hiding this comment

scottschurr Apr 26, 2021

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr commented May 28, 2021

scottschurr commented Jun 4, 2021

ximinez left a comment

Choose a reason for hiding this comment

ximinez Jun 8, 2021

Choose a reason for hiding this comment

scottschurr commented Jun 9, 2021

thejohnfreeman left a comment

Choose a reason for hiding this comment

thejohnfreeman Jun 10, 2021

Choose a reason for hiding this comment

scottschurr Jun 10, 2021

Choose a reason for hiding this comment

thejohnfreeman Jun 11, 2021

Choose a reason for hiding this comment

thejohnfreeman Jun 10, 2021

Choose a reason for hiding this comment

scottschurr commented Jun 11, 2021

ximinez left a comment

Choose a reason for hiding this comment

thejohnfreeman commented Jun 17, 2021

scottschurr commented Jun 17, 2021

mDuo13 commented Jun 18, 2021 • edited Loading

scottschurr commented Jun 21, 2021

mDuo13 commented Jun 18, 2021 •

edited

Loading