Discussion: TokenEscrowV1 #5523
Conversation
- add `sfLockedCount` to IOU trustline. (increase and decrease) - user/issuer cannot delete IOU trustline if user has `sfLockedCount` - remove `requireAuth` check in `EscrowCancel`
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5523 +/- ##
=======================================
Coverage 79.1% 79.1%
=======================================
Files 816 816
Lines 71605 71630 +25
Branches 8237 8236 -1
=======================================
+ Hits 56625 56650 +25
Misses 14980 14980
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
What's the current behavior when the escrow creator deleted their trustline and cancels their escrow? |
Woudn't the escrow object still belong to the owner's owner directory? So technically the escrow object couldn't be orphaned since the owner is obligated to delete it before they can delete their account. |
It adds the trustline back. |
| @@ -133,6 +133,9 @@ EscrowCreate::preflight(PreflightContext const& ctx) | |||
| if (!ctx.rules.enabled(featureTokenEscrow)) | |||
| return temBAD_AMOUNT; | |||
|
|
|||
| if (ctx.rules.enabled(fixTokenEscrowV1) && !ctx.tx[~sfCancelAfter]) | |||
There was a problem hiding this comment.
This seems wrong - CancelAfter does not need to be included...?
There was a problem hiding this comment.
In the original spec CancelAfter was required or discussed to be required when IOU but never implemented. This implements that requirement. If we decide we don't need that then we can remove that. I just wanted to fix my mismatch between the original spec and the implementation.
There was a problem hiding this comment.
I notice this requirement is only added for non-XRP escrows. I'm ok with that, because unlike XRP, there can be many reasons why someone wouldn't want to or may not be able to hold a given IOU, even for the time it takes to send it back to the issuer and destroy it.
I may be using "orphaned" incorrectly here. You are right, they cannot delete their account until they remove it. |
| if (!sleRippleState->isFieldPresent(sfLockedCount)) | ||
| sleRippleState->setFieldU32(sfLockedCount, 1); | ||
| else | ||
| sleRippleState->setFieldU32( | ||
| sfLockedCount, | ||
| (*sleRippleState)[~sfLockedCount].value_or(0) + 1); |
There was a problem hiding this comment.
Change sfLockedCount to soeDEFAULT, then this whole block can be written as
| if (!sleRippleState->isFieldPresent(sfLockedCount)) | |
| sleRippleState->setFieldU32(sfLockedCount, 1); | |
| else | |
| sleRippleState->setFieldU32( | |
| sfLockedCount, | |
| (*sleRippleState)[~sfLockedCount].value_or(0) + 1); | |
| (*sleRippleState)[sfLockedCount] += 1; |
(ValueProxy handles all the steps for a default field behind the scenes. += is needed only because ValueProxy does not define a ++ operator, for reasons.)
There was a problem hiding this comment.
Wow this is amazing thank you!
| auto const finalCount = | ||
| (*sleRippleState)[~sfLockedCount].value_or(0) - 1; | ||
| if (finalCount == 0) | ||
| sleRippleState->makeFieldAbsent(sfLockedCount); | ||
| else | ||
| sleRippleState->setFieldU32(sfLockedCount, finalCount); |
There was a problem hiding this comment.
Also here, if you change sfLockedCount to soeDEFAULT, then this whole block can be written as
| auto const finalCount = | |
| (*sleRippleState)[~sfLockedCount].value_or(0) - 1; | |
| if (finalCount == 0) | |
| sleRippleState->makeFieldAbsent(sfLockedCount); | |
| else | |
| sleRippleState->setFieldU32(sfLockedCount, finalCount); | |
| (*sleRippleState)[sfLockedCount] = 1; |
| if (view.rules().enabled(fixTokenEscrowV1) && | ||
| (*sleRippleState)[~sfLockedCount].value_or(0) != 0) |
There was a problem hiding this comment.
Also here, if you change sfLockedCount to soeDEFAULT, then this whole block can be written as
| if (view.rules().enabled(fixTokenEscrowV1) && | |
| (*sleRippleState)[~sfLockedCount].value_or(0) != 0) | |
| if (view.rules().enabled(fixTokenEscrowV1) && | |
| (*sleRippleState)[sfLockedCount] != 0) |
|
So I think I disagree with both "fixes" added in this amendment (as in I would be against adding them - though I could be convinced otherwise).
And an issue with authorized trustlines is equivalent to both sides being deep-frozen (the aforementioned "orphaned" escrow) which IMO should be up to the issuer and the account to figure out a resolution to. I'd rather something akin to the |
Thank you for the review. I'm happy to hear you disagree with both of the fixes because I was under the impression I had made a mistake. I agree, an EscrowClawback or InstrumentClawback is a better approach. As its not an immediate issue, I converted it to a draft and we can discuss it further. |
High Level Overview of Change
sfLockedCountto IOU trustline. (increase and decrease)sfLockedCountrequireAuthcheck inEscrowCancelContext of Change
Issues:
If an account creates an escrow, does not specify CancelAfter and then deletes their trustline the escrow object could become "orphaned".
If an IOU requires trustline authorization and the account sends their tokens back to the issuer, deletes the trustline and the adds the trustline back (in unauthorized state), the EscrowCancel transactor will fail with
tecNO_AUTHwhich leaves the escrow object "orphaned"Same for MPT, if the issuer unauthorizes the MPT, the EscrowCancel transactor will fail with
tecNO_AUTHwhich leaves the escrow object "orphaned"Solutions:
By adding the sfLockedCount onto the IOU trustline and incrementing when an escrow object is created we update the
deleteTrustlinefunction to check for a positive sfLockedCount and if there is a positive value, we reject the deletion of the trustline.1 above solves the issue of an account deleting the IOU. So they will always have an authorized trustline. For MPT we remove the requireAuth check in the EscrowCancel transactor (IOU & MPT).
removing
requireAuthcheck inEscrowCanceleffects;Note:
requireAuthis checked for both account and destination inEscrowCreateIOU: Technically this would never be possible because you cannot delete a trustline that has an escrow and you cannot unauthorize a trustline. So there is no way to get to EscrowCancel with an unauthorized trustline that was previously authorized.
MPT: On EscrowCancel the funds would return to the account, however the funds remain unauthorized.
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)