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.

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

XLS-39d: Clawback specification #104

Merged
merged 25 commits into from
May 25, 2023
Merged

XLS-39d: Clawback specification #104

merged 25 commits into from
May 25, 2023

Conversation

shawnxie999
Copy link
Collaborator

@shawnxie999 shawnxie999 commented Apr 19, 2023

Latest version: XLS-39d: Clawback

This is the XLS-39d spec proprosal for what originally is in discussion.


Proposed implementation: XRPLF/rippled#4553

@shawnxie999 shawnxie999 marked this pull request as ready for review April 19, 2023 20:00
@Silkjaer
Copy link
Contributor

Title in the filename should be lowercase and the draft should use the template format: https://github.com/XRPLF/XRPL-Standards/blob/master/xls-template.md

Also, for good measure, the author of the proposal should move this from discussion to draft, but if he is OK with it in the current form, let's progress 👍

CC: @nbougalis

@shawnxie999
Copy link
Collaborator Author

@Silkjaer Normally it would be owner of discussion to move it into PR. But this time is an exception since I will be the one updating this spec from now on

@nbougalis
Copy link

I'm fine with @shawnxie999 taking over this proposal.

@Silkjaer Silkjaer requested review from jscottbranson and removed request for intelliot April 24, 2023 18:03
@nbougalis
Copy link

nbougalis commented Apr 24, 2023

My comment is about this statement (and other similar statements in the spec):

Usually, when you want to clawback funds from a trustline, you have to freeze the trustline first, then take back the funds, and finally, unfreeze the trustline if you choose to. With these two flags, you can clawback the funds and keep the trustline unfrozen at the same time by setting both of them simultaneously.

I think that having the ability to freeze (or unfreeze) as part of the clawback makes sense and provides the issuer flexibility, but there's no real technical reason to freeze a trustline prior to executing a clawback, and you shouldn't present it as such.

@shawnxie999
Copy link
Collaborator Author

shawnxie999 commented Apr 24, 2023

I think that having the ability to freeze (or unfreeze) as part of the clawback makes sense and provides the issuer flexibility, but there's no real technical reason to freeze a trustline prior to executing a clawback. It certainly makes no sense to freeze it as part of a clawback operation, since each operation executes atomically.

@nbougalis
We spoke with stakeholders and they said they can't think of a scenario where funds are clawed back without freezing a trustline, unless we can come up with any. Normally, freezing is always a precursor of clawback in real world use case.

Also from a design perspective, it will give token holders more confidence that their funds won't be randomly clawback from them, if clawback is implemented to be part of freeze.

@nbougalis
Copy link

We spoke with stakeholders and they said they can't think of a scenario where funds are clawed back without freezing a trustline, unless we can come up with any. Normally, freezing is always a precursor of clawback in real world use case.

Perhaps operationally that's how it's done, but the spec shouldn't present this as a de-facto technical requirement.

@shawnxie999
Copy link
Collaborator Author

@nbougalis

Perhaps operationally that's how it's done, but the spec shouldn't present this as a de-facto technical requirement.

If this isn't part of the technical requirement, it will give more flexibility for the issuer. But, I think it would be more important to care about what the token holder think when it comes to the clawback ability. When this amendment goes live, token holders will be really concerned knowing that the issuer can randomly clawback all of their funds.

@shawnxie999
Copy link
Collaborator Author

I've updated the spec to add an additional lsfNoClawback flag to give more flexibility, though it's still serving as an extension of freeze. Although it would be desirable to have clawback disabled by default, it would be best to follow what has already been put in place with the default freeze settting(enabled by default). I've added an Rationale section to explain this.

@shawnxie999
Copy link
Collaborator Author

shawnxie999 commented Apr 26, 2023

Updated the spec again to reflect the change of the global flag: from lsfNoClawback to lsfAllowClawback, so that clawback is opt-out by default. We don't want to keep proposing new features that are enabled by default to increase concerns of the token holders. The existing freeze feature is an exception. From now on, any feature of this nature should be opt-out by default.

XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
@shawnxie999
Copy link
Collaborator Author

The latest revision removed the flags for clawback transaction.

XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
XLS-39d-clawback/README.md Outdated Show resolved Hide resolved
@intelliot intelliot changed the title Adding XLS-39d-Clawback specification XLS-39d: Clawback specification May 23, 2023
Copy link

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

Spec looks great! Simple and direct.

I spotted a couple of editing kinds of things that might want to be cleaned up. But nothing that should hold up release.

|------------|:----------------:|:---------:|:-------------:|
| `Amount` |:heavy_check_mark:| `object` | `AMOUNT` |

Indicates the amount being clawed back, as well as the counterparty from which the amount is being clawed back from. It is not an error if the amount exceeds the holder's balance; in that case, the maximum available balance is clawed back. It returns `temBAD_AMOUNT` is the amount is zero.

Choose a reason for hiding this comment

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

Nit: "... from which the amount is being clawed back from.
Nit: "is the amount" -> "if the amount"

This proposal introduces one new transaction: `Clawback`

#### 3.3.1. `Clawback` transaction
The **`Clawback`** transaction modifies a trustline object, by adjusting the balance accordingly and, if instructed to, by changing relevant flags. If possible (i.e. if the `Clawback` transaction would leave the trustline is in the "default" state), the transaction will also remove the trustline.

Choose a reason for hiding this comment

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

It looks like the Clawback transaction no longer affects trust line flags 🎉 . So I think the part of the sentence about "changing relevant flags" should be removed. Consider changing the sentence to something like...

The Clawback transaction modifies a trustline object by adjusting the balance. If the Clawback transaction leaves the trustline in the "default" state, the transaction also removes the trustline.

@Silkjaer Silkjaer merged commit 66080b5 into XRPLF:master May 25, 2023
@shortthefomo
Copy link
Collaborator

This really is the first issue that has highlighted the interdependency that is some times present in tickets for me, this proposal has in its discussion phase treaded lightly on the fact that it effects others.

I would like to understand the rational given that the AMM was excluded from the scope of this? Yet other non-spendable transaction types like Escrows (for IOU's XLS-34) are not. Both are at similar stage of completed.

Possibly going forward, a review of what amendments are effected, proposed as well us the current working body of work should be one of the agreed upon gates before the XLS gets closed.

@shawnxie999
Copy link
Collaborator Author

@lathanbritz Perhaps the spec was not clear. Payment channel and escrow of IOU is excluded from as well. In other words, clawback cannot clawback the tokens that are locked up in escrow or payment channel. It would not affect other amendments

@shortthefomo
Copy link
Collaborator

Thank you @shawnxie999 maybe the review process could specifically require a effected/not-effected table as the XLS agreed upon and closed, would be very helpful.

@shawnxie999
Copy link
Collaborator Author

maybe the review process could specifically require a effected/not-effected table as the XLS agreed upon and closed, would be very helpful.

I think that's a good idea. Should we update the template to introduce a new section for this? @intelliot

@intelliot intelliot mentioned this pull request Jun 26, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants