`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

• [x] 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: https://github.com/XRPLF/XRPL-Standards/discussions/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.

[intelliot]

@dangell7 what's the next step for this PR? Would you like to request for it to be code reviewed?

[dangell7]

@dangell7 what's the next step for this PR? Would you like to request for it to be code reviewed?

Yes I would. I thought it was in the queue tbh... There are 2 other additional functions you/I have referenced that I can attempt to include but it would be awesome if we could get an initial review done. Then we can discuss the other 2 functionalities.

I will bring this up to date and fix the merge conflicts.

[intelliot]

When this is ready for review, please comment on this PR to say something to the effect of, "This is now ready for review." This informs all viewers that you don't have any additional pending changes in progress.

[dangell7]

@intelliot I dropped the additional features. We can discuss them in an XLS after this PR is reviewed.

"This is now ready for review."

[danielwwf]

Just wanted to check in and see whats the plan is with this one. Not sure how many people would use it but we have 2 apps that can utilize it, potentially 12k+ users, and building a third one atm that requires escrow. Having TokenEscrow at our disposal would boost the usage.

@scottschurr @kennyzlei @gregtatcam @nbougalis @seelabs @ledhed2222 @shawnxie999

[kennyzlei]

@shawnxie999 and @ledhed2222 will be able to help with an initial review in about a week or so. would be great to get additional help from the community to review this as well!

[RichardAH]

@shawnxie999 thank you for the code review. You are correct that seoREQUIRED will cause problems. Denis will make a change now.

[dangell7]

@shawnxie999 I'm having an issue building locally so I just dumped everything and I have to nuke my computer.

I'm not finished making your updates. I will make the thumbs up as soon as I can build locally again.

[intelliot]

@dangell7 - please confirm when your updates are complete and have been pushed to this PR

[dangell7]

@intelliot Shawn was the last one to review and he approved.

I didn't see these merge conflicts and will address them but were just waiting for another reviewer...

[RichardAH]

We're closing this, for now. It will go on the Hooks sidechain instead. Perhaps it can go on mainnet in the future.

[intelliot]

@RichardAH said it's OK to reopen this PR.

Previous comment from @dangell7 :

@shawnxie999 I'm having an issue building locally so I just dumped everything and I have to nuke my computer.

I'm not finished making your updates. I will make the thumbs up as soon as I can build locally again.

I assume those updates were finished 🤔

@dangell7 wrote:

I didn't see these merge conflicts and will address them but were just waiting for another reviewer...

Can you address the merge conflicts?

[dangell7]

@intelliot To my understanding all current issues and merge conflicts have been resolved.

[ximinez]

I started an idea / pre-standard proposal over at https://github.com/XRPLF/XRPL-Standards/discussions/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

https://github.com/XRPLF/XRPL-Standards/discussions/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[bot]]

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).

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	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%)	:arrow_up:
src/xrpld/rpc/handlers/AccountChannels.cpp	82.0% <100.0%> (+0.2%)	:arrow_up:
... and 9 more

... and 10 files with indirect coverage changes