PaychanAndEscrowForTokens: Token-Enabled Escrows and Payment Channels support (XLS-34d+)

[dangell7]

High Level Overview of Change

The proposed amendment to the XRPL protocol, XLS-34d: Token-Enabled Escrows and Payment Channels, would introduce changes to the ledger objects, transactions, and rpc methods to enable Escrows and PayChannels to use Trustline balances.

Context of Change

The proposed changes would include modifications to the following ledger objects;

RippleState
PayChannel
Escrow

The proposed changes would include modifications to the following transactions;

PaymentChannelCreate
PaymentChannelFund
PaymentChannelClaim
EscrowCreate

The proposed changes would include modifications to the following RPC methods;

account_lines
account_channels
account_objects
channel_authorize
channel_verify

Type of Change

• New feature (non-breaking change which adds functionality)

Test Plan

PaychanAndEscrowForTokens unit tests are added for all core functionality features.

Future Tasks

[RichardAH]

Wondering if @ximinez and/or @scottschurr might be able to do a review of this amendment

[ximinez]

Wondering if @ximinez and/or @scottschurr might be able to do a review of this amendment

I'd love to, but it'll be some time before I can get to it.

[intelliot]

For context, what is motivating this feature addition? Who will use it?

[RichardAH]

For context, what is motivating this feature addition? Who will use it?

There's a problem statement in the XLS here: XRPLF/XRPL-Standards#88 copied below for reference

The XRPL supports several types of on-ledger negotiable Instruments, namely: Escows, PayChannels and Checks. While each of these instruments is implemented as a first-class on-ledger object, only the Check object supports the use of Trustline balances. PayChannels and Escrows support only the native asset XRP. This limitation is a barrier to wider-spread use of these Instruments for reasons including:

1. Regulatory compliance.
2. Unwillingness to hold a counter-party-free asset (i.e. XRP).
3. Volatility and exchange-rate risk.

[ximinez]

I started an idea / pre-standard proposal over at XRPLF/XRPL-Standards#133

I know that what we have in this PR is a fully functional implementation, but I have serious qualms about it (outlined at the above link), that I would like to discuss before this PR moves forward.

This is not a veto. This solution might be the right way to go, but I'm not convinced of that.

[RichardAH]

Just a PSA: We're not actively promoting or defending XLS34 for merge into mainnet. We think it's the correct approach and it's what we're using on Xahau. If Ripple engineers want to (or don't want to) merge it into mainnet that's completely up to them.

[outrum]

Great work on XLS-34d! This functionality is essential for most XRPL tokens, in my view. Appreciating the complexities involved, I just wanted to express my support and eagerness to see this feature go live. Your efforts in pushing this forward are invaluable. Thanks for all the hard work!

[dangell7]

This PR has been updated to reflect the concerns address by @ximinez

XRPLF/XRPL-Standards#133

• trustAdjustLockedBalance -> transferToEntry: The balance is no longer "locked" on the trustline, but the sfAmount is "moved" from the source and stored on the entry object.
• trustTransferLockedBalance -> transferFromEntry: The LockedBalance is no longer transferred from LockedBalance, but now the sfAmount on the entry object is "moved" from the entry object to the destination.
• LockedBalance is removed
• LockedCount is removed

[codecov]

Codecov Report

Attention: Patch coverage is 83.38509% with 107 lines in your changes missing coverage. Please review.

Project coverage is 71.4%. Comparing base (f3bcc65) to head (1806df0).
Report is 41 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/ledger/View.h	78.1%	37 Missing ⚠️
src/xrpld/app/tx/detail/PayChan.cpp	85.7%	27 Missing ⚠️
src/xrpld/app/tx/detail/Escrow.cpp	87.1%	16 Missing ⚠️
src/xrpld/ledger/detail/View.cpp	65.4%	9 Missing ⚠️
include/xrpl/protocol/STAmount.h	73.1%	7 Missing ⚠️
include/xrpl/protocol/PayChan.h	68.8%	5 Missing ⚠️
src/xrpld/net/detail/RPCCall.cpp	89.5%	4 Missing ⚠️
src/xrpld/rpc/handlers/PayChanClaim.cpp	93.8%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #4396     +/-   ##
=========================================
+ Coverage     71.3%   71.4%   +0.1%     
=========================================
  Files          796     796             
  Lines        67031   67582    +551     
  Branches     10885   10899     +14     
=========================================
+ Hits         47808   48281    +473     
- Misses       19223   19301     +78     

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	100.0% <ø> (ø)
include/xrpl/protocol/TER.h	100.0% <ø> (ø)
include/xrpl/protocol/UintTypes.h	100.0% <100.0%> (ø)
src/libxrpl/protocol/Feature.cpp	94.6% <ø> (ø)
src/libxrpl/protocol/Indexes.cpp	97.6% <100.0%> (ø)
src/libxrpl/protocol/LedgerFormats.cpp	100.0% <ø> (ø)
src/libxrpl/protocol/SField.cpp	77.5% <ø> (ø)
src/libxrpl/protocol/TER.cpp	100.0% <ø> (ø)
src/xrpld/app/tx/detail/InvariantCheck.cpp	87.7% <100.0%> (+0.7%)	⬆️
src/xrpld/rpc/handlers/AccountChannels.cpp	82.0% <100.0%> (+0.2%)	⬆️
... and 9 more

... and 10 files with indirect coverage changes

[ximinez]

This PR has been updated to reflect the concerns address by @ximinez

XRPLF/XRPL-Standards#133

* `trustAdjustLockedBalance` -> `transferToEntry`: The balance is no longer "locked" on the trustline, but the `sfAmount` is "moved" from the source and stored on the entry object.
* `trustTransferLockedBalance` -> `transferFromEntry`: The `LockedBalance` is no longer transferred from `LockedBalance`, but now the `sfAmount` on the entry object is "moved" from the entry object to the destination.
* `LockedBalance` is removed
* `LockedCount` is removed

I appreciate the work you've been doing here. Unfortunately, I haven't had a chance to review the code changes themselves, and probably won't for a while. I dug up the message I sent you a while back where I outlined my ideas for the steps needed to implement token support:

1. A one-way account flag for issuers to opt in to the feature.
2. Add an optional sfIssuerNode field to the Escrow and Paychan Objects - similar to the sfOwnerNode and sfDestinationNode.
3. sfAmount is already the right type - the only restrictions on it being an IOU are in the logic, so change that logic.
4. When an Escrow/PayChan is created, subtract the amount from the TrustLine, and set the amount in the new object - just like we do with XRP, except that it would probably need to account for rounding, Unlike a TrustLine, the amount will always be positive. There's no need to worry about high/low, because the abstraction is different (Owner, Destination, Issuer). Also, add the new object to the Issuer's directory.
5. When the Escrow/PayChan is redeemed, move the funds the other way around. If the TrustLine doesn't exist, it can only be created if the recipient is the one the initiated the transaction.

From your description here, it looks like you've implemented items 3, 4, and maybe 5. That leaves items 1 and 2. Where are you on those?

Also, what do you think about splitting these changes into two PRs: One for Escrows and one for Payment Channels to possibly make code review easier?

[dangell7]

This PR has been updated to reflect the concerns address by @ximinez
XRPLF/XRPL-Standards#133

* `trustAdjustLockedBalance` -> `transferToEntry`: The balance is no longer "locked" on the trustline, but the `sfAmount` is "moved" from the source and stored on the entry object.
* `trustTransferLockedBalance` -> `transferFromEntry`: The `LockedBalance` is no longer transferred from `LockedBalance`, but now the `sfAmount` on the entry object is "moved" from the entry object to the destination.
* `LockedBalance` is removed
* `LockedCount` is removed

I appreciate the work you've been doing here. Unfortunately, I haven't had a chance to review the code changes themselves, and probably won't for a while. I dug up the message I sent you a while back where I outlined my ideas for the steps needed to implement token support:

1. A one-way account flag for issuers to opt in to the feature.
2. Add an optional sfIssuerNode field to the Escrow and Paychan Objects - similar to the sfOwnerNode and sfDestinationNode.
3. sfAmount is already the right type - the only restrictions on it being an IOU are in the logic, so change that logic.
4. When an Escrow/PayChan is created, subtract the amount from the TrustLine, and set the amount in the new object - just like we do with XRP, except that it would probably need to account for rounding, Unlike a TrustLine, the amount will always be positive. There's no need to worry about high/low, because the abstraction is different (Owner, Destination, Issuer). Also, add the new object to the Issuer's directory.
5. When the Escrow/PayChan is redeemed, move the funds the other way around. If the TrustLine doesn't exist, it can only be created if the recipient is the one the initiated the transaction.

From your description here, it looks like you've implemented items 3, 4, and maybe 5. That leaves items 1 and 2. Where are you on those?

Also, what do you think about splitting these changes into two PRs: One for Escrows and one for Payment Channels to possibly make code review easier?

You're right I did skip 1 & 2 but that shouldn't be too hard to implement. Will add that now.

I can split them into 2 PR's no problem. I will go ahead and do that now as well. Thanks for taking a look.