Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-39 Clawback: #4553
XLS-39 Clawback: #4553
Changes from 1 commit
eeeb90c
01f7c69
e17e3cf
5309c6b
f4d1d90
6981d5d
113ca51
43fb0aa
2752b9b
d5e1e92
3f32409
6ea5b73
4517eaa
2f536f3
5f54117
6d5dd3e
0c0c57a
b2acaaa
e49e581
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anstd::optional<STAmount>
; or (b) by returning a more generic error code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 tofhIGNORE_FREEZE
, calledfhINCLUDE_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 inaccountHolds
when/if it gets merged.Also, I think that
tecINSUFFICIENT_FUNDS
is a better error code here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code to clear the
lsfAllowClawback
flag (i.e. to disclaim your ability to clawback) is missing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this flag is never meant to be cleared. Once set, it cannot be cleared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why? This is the inverse of "no freeze" and it should definitely be a flag you can clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
andNoFreeze
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going on a tangent here: Since
Clawback
andNoFreeze
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 anAccountSet
.