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-85d Token Escrow #272

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

XLS-85d Token Escrow #272

wants to merge 9 commits into from

Conversation

dangell7
Copy link
Contributor

No description provided.


- **MPTs**:
- **Lock Conditions (Deep Freeze Equivalent)**: The transaction succeeds, allowing the escrow to be cancelled.
- **Transfer Flags**: If `tfMPTCanTransfer` is not enabled, transaction fails with `tecNO_PERMISSION`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

My thinking is that cancellation should not need to check for can transfer. Here's my rationale, but pressure test it:

First, the tfMPTCanTransfer is an immutable flag (i.e., only settable on creation of an MPT issuance and not changeable later). So, there's a case to be made that it should not have been possible to create an escrow in the first place if this flag is false on an issuance (except possibly when an escrow is created with source + destination as the same address).

If we accept that functionality, then cancel shouldn't need to check for can transfer flag because either the escrow wouldn't have gotten created, or cancel is just fine since it's going back to the original sender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is interesting: (except possibly when an escrow is created with source + destination as the same address).

Yes I agree, you should not need it. I will remove it.

Copy link
Collaborator

@mvadari mvadari Mar 10, 2025

Choose a reason for hiding this comment

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

I'm curious what you guys think should happen here if this is an escrow to yourself. You're not actually transferring tokens to anybody then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a good point @mvadari -- though at the same I can't really come up with a good reason to disallow this.

Plus, current XRP escrows allow escrowing to oneself, so making tokens do something different feels like violating the principle of least surprise.

Also, perhaps being able to demonstrate some sort of "lock up" of one's own funds is one use-case? Ripple escrows come to mind, so in this context maybe putting funds on hold for a time has some value for a business contract or otherwise. Or, maybe something more simple just like, "I decided to put some MPT into an escrow so I don't spend it for 6 months because I'm not great at spending control"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RE Issuer to Issuer (Current Implementation)

IOU: Nothing happens on trustlines. Escrow LE is created and escrowed would show in the gateway_balances
MPT: The EscrowedAmount on the issuance increases, outstanding does not change.

You can see the issuer tests here: https://github.com/XRPLF/rippled/blob/528f9d5bee411a2131cf3cc91b0aae593492371b/src/test/app/EscrowToken_test.cpp#L1512

RE Self to Self

I'm not sure anything special needs to occur. I will create a test case for it though.


- **Transfer from Source to Issuer Holding:**
- **IOU Tokens**: Amount deducted from the source's trustline balance.
- **MPTs**: Amount moved to a holding controlled by the issuer; the issuer's total outstanding amount remains unchanged.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there's any kind of amount held by the issuer in the MPT case. I think the value just goes into the escrow. But clarify this one if I'm misunderstanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will have to review the rippleCredit function again. I thought the funds go back to the issuer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wording could be changed though. "A holding controlled by the issuer" isn't great haha

Copy link
Collaborator

@sappenin sappenin Mar 10, 2025

Choose a reason for hiding this comment

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

I will have to review the rippleCredit function again. I thought the funds go back to the issuer.

Ah, I see -- I was imagining a different design where rippleCredit is not used (I've looked more closely at the current code PR so I'm tracking a bit better now).

So, I see that the current implementation is taking MPT from the sender, and sending it back to the MPT issuer via here (i.e., the issuer is specified as the destination). However, I'm uncertain about this design for two reasons. First, MPT issuers aren't allow to hold MPTs like regular accounts can hold MPT (MPT issuers don't have MPToken objects). This means that when a normal MPT payment is made to an issuer, it's actually just a redeem operation, and outstandingAmount is the thing that gets reduced. However, in the case of an EscrowCreate, we don't want to mutate the outstandingAmount. Second, it's unclear what value trying to have an MPT Issuer hold a temporary MPToken when we can simply store the Escrowed MPT amount directly on the Escrow object.

So, for MPT escrow, I wonder if we just do something similar to what happens for XRP (i.e., reduce the sender, and increase the amount on the escrow).

This raises another interesting question: Is it acceptable for an MPT issuer to create an escrow? I think this should be fine, but I need to think a bit more about that one.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, in the case of MPT rippleCredit there are 4 paths.

  1. uSenderID == issuer -> outstanding amount increased
  2. uSenderID != issuer -> sender MPTAmount = amt - pay
  3. uReceiverID == issuer -> outstanding amount decreased
  4. uReceiverID != issuer -> MPTAmount += amt

So no, the tokens do not move back to the issuer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed that's how rippleCredit works. But the Escrow::doApply in your implementation here is sending escrowed funds back to the issuer (that's item #3 in your list, unless I'm misreading your implementation?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No you're correct. I was stuck on "sending escrowed funds" vs an adjustment to outstanding.

I have updated the code. Added a function called rippleEscrow. This now updates a new field sfEscrowedAmount and doesn't adjust sfOutstandingAmount.

I was going to add a bool to rippleCredit but I think a seperate function is better so that I dont have to guard existing functionality with a rule and it can be used later and maybe its more intuitive.

XRPLF/rippled@3e029b6

**Failure Conditions:**

1. **Source Not Authorized to Hold Token:**
- If authorization is required and the source is not authorized, transaction fails with `tecNO_AUTH`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there anything we can do to mitigate this condition? This feels like a stuck escrow (which may be unavoidable, but would be nice to avoid). In this case, the sender may not be able to rectify the situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I think if the holder is not authorized and there is require auth then allowing them to hold the token violates the require auth.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the holder is not authorized and there is require auth then allowing them to hold the token violates the require auth.

Looking into this for MPTs at least, if an account has a non-zero balance of MPT, then it can't be unauthorized. The issuer would have to Clawback all MPT first, and then un-authorize.

I think we should strive for the same behavior here (i.e., if an issuer is trying to unauthorize an account that has MPT in escrow, then this transaction should fail). Primary reason is it's very inelegant to have functionality work differently across features.

To pull this off though (at least for MPT), we'd need to perhaps track the total amount of MPT in-escrow on the MPToken object of the sender (or a boolean maybe like hasEscrow?). Alternatively, the transaction check would have to iterate through an account's escrows, checking for any MPT that matches the issuer doing the un-auth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could store the escrowed balance on the MPT. So the MPTAmount is 0 but the sfEscrowAmount != 0.

Done here: XRPLF/rippled@3e029b6

@theotherian
Copy link

theotherian commented Mar 7, 2025

Any chance this would be added to the gateway_balances resource to avoid having to make two trips to the ledger to find out the total supply at an issuer?

@dangell7
Copy link
Contributor Author

dangell7 commented Mar 7, 2025

Any chance this would be added to the gateway_balances resource to avoid having to make two trips to the ledger to find out the total supply at an issuer?

#272 (comment)


- **Transfer from Source to Issuer Holding:**
- **IOU Tokens**: Amount deducted from the source's trustline balance.
- **MPTs**: Amount moved to a holding controlled by the issuer; the issuer's total outstanding amount remains unchanged.
Copy link
Collaborator

@sappenin sappenin Mar 10, 2025

Choose a reason for hiding this comment

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

I will have to review the rippleCredit function again. I thought the funds go back to the issuer.

Ah, I see -- I was imagining a different design where rippleCredit is not used (I've looked more closely at the current code PR so I'm tracking a bit better now).

So, I see that the current implementation is taking MPT from the sender, and sending it back to the MPT issuer via here (i.e., the issuer is specified as the destination). However, I'm uncertain about this design for two reasons. First, MPT issuers aren't allow to hold MPTs like regular accounts can hold MPT (MPT issuers don't have MPToken objects). This means that when a normal MPT payment is made to an issuer, it's actually just a redeem operation, and outstandingAmount is the thing that gets reduced. However, in the case of an EscrowCreate, we don't want to mutate the outstandingAmount. Second, it's unclear what value trying to have an MPT Issuer hold a temporary MPToken when we can simply store the Escrowed MPT amount directly on the Escrow object.

So, for MPT escrow, I wonder if we just do something similar to what happens for XRP (i.e., reduce the sender, and increase the amount on the escrow).

This raises another interesting question: Is it acceptable for an MPT issuer to create an escrow? I think this should be fine, but I need to think a bit more about that one.

Thoughts?

- `CancelAfter`: When the Escrow Expires (Required on IOU/MPT)
- `Amount`: Tokens held in escrow.
- `TransferRate`: `TransferRate` (IOUs) or `TransferFee` (MPTs) at creation.
- `IssuerNode`: Reference to the issuer’s ledger node if applicable.

Choose a reason for hiding this comment

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

Is IssuerNode a pointer to the issuer's AccountRoot entry, or does it point to something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It points to the page where the escrow keylet was stored in the issuer directory

Choose a reason for hiding this comment

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

Ah ok, that was actually my next question -- it sounds like all Escrows involving an issuer have their keylets stored in the issuer's owner directory, is that correct? Going back to this thread, I think this means you could compute the total amount of IOUs issued by an issuer by iterating over the issuer's owner directory, summing up all RippleState balances and Escrowed amounts. Is my understanding correct there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if the issuer is the sender or the receiver there is a keylet stored in the issuers directory.

As for the issuer's escrowed amount I'm not certain of the exact implementation until I write it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dangell7
Copy link
Contributor Author

Update the spec.

  • Cleaned up some redundancy
  • Added extra information for StateChanges (OutstandingAmount & EscrowedAmount)
  • Added failure for Issuer as Source
  • Added Future Considerations
  • Added Update to Ledger Entry Objects

Open Issues:

Nomenclature for sfEscrowedAmount

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.

5 participants