XLS-39 Clawback: #4553

shawnxie999 · 2023-05-31T14:03:43Z

High Level Overview of Change

Introduces:

AccountRoot flag: ~~lsfAllowClawback~~ lsfAllowTrustLineClawback
- (renamed by Rename allowClawback flag to allowTrustLineClawback #4617)
New Clawback transaction

Context

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

nbougalis

Left some comments that need to be addressed before I give this one a pass.

Also, I notice that there no invariant checkers where added. At least one should be possible to add (see one of the comments in this review).

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

nbougalis · 2023-06-01T04:55:23Z

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

+    STAmount const balance = sleRippleState->getFieldAmount(sfBalance);
+
+    // If balance is positive, issuer must have higher address than holder
+    if (balance > beast::zero && issuer < holder)
+        return tecNO_PERMISSION;
+
+    // If balance is negative, issuer must have lower address than holder
+    if (balance < beast::zero && issuer > holder)
+        return tecNO_PERMISSION;


You're doing precisely what you advise should not be done in your comment a few lines down (starting on line 89).

I think the better path forward is to add a flag to accountHolds similar to fhIGNORE_FREEZE, called fhINCLUDE_ESCROW which instructs that function to return the full balance, including any escrowed amounts (since, IMO, the issuer should be able to clawback escrowed amounts), and leaving it up to the IOU escrow code to handle the situation in accountHolds when/if it gets merged.

Also, I think that tecINSUFFICIENT_FUNDS is a better error code here.

This if-condition is purely used to check that the account of this transaction is the issuer, by comparing high/low account with the polarity of the balance. It's not used to check the balance.

Also, if we want to be able to claw back escrowed amounts, I think it would be better to propose a separate amendment after both PRs are merged. I'm don't think it is a good idea to have two PRs making changes for it in parallel

I don't think escrow or pay channels should be included in the clawback if AMM is excluded.

The justification is "it only allows an issuer to claw back the funds that are spendable".

Escrowed or Paychan iou tokens are locked and are not spendable. Therefore escrow and paychannel should be excluded.

nbougalis · 2023-06-01T04:59:28Z

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

+    auto const sleRippleState =
+        ctx.view.read(keylet::line(holder, issuer, clawAmount.getCurrency()));
+    if (!sleRippleState)
+        return tecNO_LINE;


Loading a SLE is an expensive operation, and you could avoid it, since accountHolds will also do it. Of course, it won't tell you if the trust line doesn't exist, but you could (a) either improve the interface of the (legacy) accountHolds to return an std::optional<STAmount>; or (b) by returning a more generic error code.

The SLE of the trustline is fetched to check the account must be the issuer in the trustline. Without loading it and purely by using accountHolds, I would not be able to know that

// If balance is positive, issuer must have higher address than holder if (balance > beast::zero && issuer < holder) return tecNO_PERMISSION; // If balance is negative, issuer must have lower address than holder if (balance < beast::zero && issuer > holder) return tecNO_PERMISSION;

i would like to explicitly check the high/low account and the balance polarity to verify that the issuer is the one who submitted this transaction. Even though checking the sign of what accountHolds returns could work too, but that is not the purpose of this function, and behaviors could possibly change too. I think making an extra call is worth it in this case, because I would like to be fully confident that the real issuer is the one submitting this transaction, not the token holder.

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

nbougalis · 2023-06-01T05:47:54Z

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

+    if (ctx_.view().rules().enabled(featureClawback) &&
+        uSetFlag == asfAllowClawback)
+    {
+        JLOG(j_.trace()) << "set allow clawback";
+        uFlagsOut |= lsfAllowClawback;
+    }


The code to clear the lsfAllowClawback flag (i.e. to disclaim your ability to clawback) is missing.

this flag is never meant to be cleared. Once set, it cannot be cleared

But why? This is the inverse of "no freeze" and it should definitely be a flag you can clear.

@nbougalis The reason for making clawback opt-out by default was purely because, having clawback opt-in was a poor idea and would raise a lot of concerns, just like how having freeze opt-in by default was a bad choice. The verdict is that we would like "opt-out" these kind of regulatory features in the future, and clawback would be the first one to follow that.

What is really the incentive for making allow clawback a mutable field? If they really want to mutate it, then I don't see why can't they create a new account. It would also create unnecessary complexity and confusion as well, since AllowClawback and NoFreeze are interdependent on each other.

But I think it would be a good practice to stick with immutable fields for regulatory features like freeze or clawback.

No, you misunderstand: I'm not suggesting that clawback should be opt-out, that would be a bad idea. I was just saying that I would allow it to be unset, but I don't feel strongly enough about it.

I'm not quite sure if there is any need for this. But if we find out that unsetting the flag is really wanted somehow, I think it would be fine to add it through an amendment later. This would be a rather small change. But I think it would be a good idea to keep it simple for now.

Going on a tangent here: Since Clawback and NoFreeze are complimentary to one another, is it a good idea to frame this condition as an invariant? I would eliminate such a check in other parts of the code right?

@ckeshava I don't think this change would eliminate such checks in other parts of the code. Invariant check is executed after a transaction.

And it will still be mandatory to check this complementary property during the preclaim (before an transaction) of an AccountSet.

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

src/ripple/protocol/TxFlags.h

shawnxie999 · 2023-06-02T21:02:58Z

Ready for re-review

seelabs

👍

shawnxie999 · 2023-06-15T22:56:26Z

@nbougalis do you mind revisiting the PR to see it looks acceptable? Thanks

intelliot · 2023-06-21T21:53:26Z

(note: nbougalis intends to review by this weekend)

* Update the `account_info` API so that the `allowClawback` flag is included in the response. * The proposed `Clawback` amendement added an `allowClawback` flag in the `AccountRoot` object. * In the API response, under `account_flags`, there is now an `allowClawback` field with a boolean (`true` or `false`) value. * For reference, the XLS-39 Clawback implementation can be found in #4553 Fix #4588

Introduces: * AccountRoot flag: lsfAllowClawback * New Clawback transaction * More info on clawback spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-39d-clawback

* Update the `account_info` API so that the `allowClawback` flag is included in the response. * The proposed `Clawback` amendement added an `allowClawback` flag in the `AccountRoot` object. * In the API response, under `account_flags`, there is now an `allowClawback` field with a boolean (`true` or `false`) value. * For reference, the XLS-39 Clawback implementation can be found in XRPLF#4553 Fix XRPLF#4588

Introduces: * AccountRoot flag: lsfAllowClawback * New Clawback transaction * More info on clawback spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-39d-clawback

* Update the `account_info` API so that the `allowClawback` flag is included in the response. * The proposed `Clawback` amendement added an `allowClawback` flag in the `AccountRoot` object. * In the API response, under `account_flags`, there is now an `allowClawback` field with a boolean (`true` or `false`) value. * For reference, the XLS-39 Clawback implementation can be found in XRPLF#4553 Fix XRPLF#4588

Introduces: * AccountRoot flag: lsfAllowClawback * New Clawback transaction * More info on clawback spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-39d-clawback

* Update the `account_info` API so that the `allowClawback` flag is included in the response. * The proposed `Clawback` amendement added an `allowClawback` flag in the `AccountRoot` object. * In the API response, under `account_flags`, there is now an `allowClawback` field with a boolean (`true` or `false`) value. * For reference, the XLS-39 Clawback implementation can be found in XRPLF#4553 Fix XRPLF#4588

shawnxie999 changed the title ~~Clawback support~~ XLS-39 Clawback May 31, 2023

shawnxie999 changed the title ~~XLS-39 Clawback~~ XLS-39 Clawback: May 31, 2023

shawnxie999 force-pushed the clawback branch 2 times, most recently from 9af831d to 4b3c4d7 Compare May 31, 2023 15:32

shawnxie999 mentioned this pull request May 31, 2023

Clawback shawnxie999/rippled#4

Closed

7 tasks

shawnxie999 marked this pull request as ready for review May 31, 2023 16:01

Clawback support

eeeb90c

shawnxie999 force-pushed the clawback branch from 5689ae4 to eeeb90c Compare May 31, 2023 16:31

kennyzlei requested review from seelabs, ledhed2222 and ckeshava May 31, 2023 18:06

kennyzlei added the Amendment label May 31, 2023

nbougalis suggested changes Jun 1, 2023

View reviewed changes

shawnxie999 added 5 commits June 1, 2023 10:19

remove redundant check and comment

01f7c69

change error code to insufficient fund

e17e3cf

add invariant check

5309c6b

clang

f4d1d90

plural variable

6981d5d

ckeshava reviewed Jun 1, 2023

View reviewed changes

src/ripple/app/tx/impl/Clawback.cpp Show resolved Hide resolved

ckeshava reviewed Jun 1, 2023

View reviewed changes

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

shawnxie999 added 6 commits June 2, 2023 10:13

amm maybe used

113ca51

freeze logic to preclaim

43fb0aa

clang

2752b9b

unused var

d5e1e92

make clawback inline

3f32409

clang

6ea5b73

ckeshava reviewed Jun 2, 2023

View reviewed changes

src/ripple/protocol/TxFlags.h Show resolved Hide resolved

shawnxie999 requested a review from nbougalis June 2, 2023 21:03

modify invaraint

4517eaa

shawnxie999 added 2 commits June 15, 2023 12:22

address comments

6d5dd3e

clang

0c0c57a

seelabs approved these changes Jun 15, 2023

View reviewed changes

intelliot assigned nbougalis Jun 16, 2023

Merge remote-tracking branch 'upstream/develop' into clawback

b2acaaa

nbougalis approved these changes Jun 26, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into clawback

e49e581

kennyzlei unassigned nbougalis Jun 26, 2023

kennyzlei added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 26, 2023

kennyzlei merged commit b7e902d into XRPLF:develop Jun 26, 2023
15 checks passed

intelliot added this to the 1.12 milestone Jun 26, 2023

intelliot added API Change Will Need Documentation labels Jun 27, 2023

This was referenced Jun 28, 2023

Add allowClawback flag for account_info #4590

Merged

allowClawback flag is missing from the account_info.account_flags response (Version: 1.12.0-beta1) #4588

Closed

intelliot added a commit to intelliot/rippled that referenced this pull request Jul 18, 2023

API-CHANGELOG - Clawback (XRPLF#4553, XRPLF#4617)

ec67941

intelliot added the Added to API Changelog label Jul 25, 2023

intelliot mentioned this pull request Jul 13, 2023

Support XLS-39 Clawback XRPLF/clio#767

Closed

intelliot added the Clio Reviewed label Aug 1, 2023

intelliot pushed a commit that referenced this pull request Aug 16, 2023

XLS-39 Clawback: (#4553)

3bd4998

Introduces: * AccountRoot flag: lsfAllowClawback * New Clawback transaction * More info on clawback spec: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-39d-clawback

amarantha-k mentioned this pull request Sep 6, 2023

Porting Clawback documentation to xrpl.org XRPLF/xrpl-dev-portal#2111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XLS-39 Clawback: #4553

XLS-39 Clawback: #4553

shawnxie999 commented May 31, 2023 •

edited by intelliot

nbougalis left a comment

nbougalis Jun 1, 2023

shawnxie999 Jun 1, 2023

shawnxie999 Jun 1, 2023

dangell7 Jun 1, 2023

nbougalis Jun 1, 2023

shawnxie999 Jun 1, 2023

shawnxie999 Jun 1, 2023 •

edited

nbougalis Jun 1, 2023

shawnxie999 Jun 1, 2023

nbougalis Jun 1, 2023

shawnxie999 Jun 1, 2023 •

edited

nbougalis Jun 1, 2023

shawnxie999 Jun 2, 2023 •

edited

ckeshava Jun 9, 2023

shawnxie999 Jun 12, 2023 •

edited

shawnxie999 commented Jun 2, 2023 •

edited

seelabs left a comment

shawnxie999 commented Jun 15, 2023

intelliot commented Jun 21, 2023

XLS-39 Clawback: #4553

XLS-39 Clawback: #4553

Conversation

shawnxie999 commented May 31, 2023 • edited by intelliot

High Level Overview of Change

Context

Type of Change

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnxie999 Jun 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnxie999 Jun 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnxie999 Jun 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnxie999 Jun 12, 2023 • edited

Choose a reason for hiding this comment

shawnxie999 commented Jun 2, 2023 • edited

seelabs left a comment

Choose a reason for hiding this comment

shawnxie999 commented Jun 15, 2023

intelliot commented Jun 21, 2023

shawnxie999 commented May 31, 2023 •

edited by intelliot

shawnxie999 Jun 1, 2023 •

edited

shawnxie999 Jun 1, 2023 •

edited

shawnxie999 Jun 2, 2023 •

edited

shawnxie999 Jun 12, 2023 •

edited

shawnxie999 commented Jun 2, 2023 •

edited