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-0066d Lending Protocol #240

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

Conversation

Tapanito
Copy link
Contributor

Discussion thread can be found here: #190
Development branch can be found here: TBD

@sappenin
Copy link
Collaborator

@Tapanito this PR seems ready to merge -- do you agree?

(Note that per the DRAFTS section of the CONTRIBUTING guidelines, merging does not mean endorsement or even a completed spec, but instead offers a way for spec editors to more easily edit using PRs).

@Tapanito
Copy link
Contributor Author

Tapanito commented Dec 4, 2024

Before merging, I'd like to have at least one review by an engineer to ensure everything is in order. In addition, similarly to Single Asset Vault, it is simpler/more convenient to edit a PR with various small design changes (e.g. names), rather than open a new PR for each of them. As a result, keeping a PR open allows to have a spec. that is more readily in-alignment with implementation.

@sappenin
Copy link
Collaborator

sappenin commented Dec 4, 2024

Makes sense, especially if it's more convenient. Just didn't want you (or anyone reading along) to think that merging here implies "finished product" (that's why we have a d designator, to indicate draft status).

Tapanito and others added 7 commits January 27, 2025 16:40
- Adds VaultNode to LoanBroker object to track in which owner directory of the Vaults pseudo-account the LoanBroker object is referenced.
- Adds LoanBrokerNode to Loan object to track in which owner directory of the LoanBroker object the Loan is references.
- Replaces CurrentTime to LastClosedLedger.CloseTime.
- Changes the LoanBroker.Delete transaction to automatically return any outstanding Cover to the LoanBroker.Owner.
- Adds a balance check to the LoanBrokerCoverDeposit transaction when depositing XRP.
- Adds a check to LoanBrokerCoverWithdraw to ensure the CoverAvailable does not drop below Mimimum Cover Required.
@Tapanito
Copy link
Contributor Author

Tapanito commented Jan 28, 2025 via email

@Tapanito
Copy link
Contributor Author

Tapanito commented Jan 28, 2025 via email

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Updates look good.

| ----------------- | :----------------: | :-------: | :-----------: | :-----------: | :------------------------------------------------ |
| `TransactionType` | :heavy_check_mark: | `string` | `UINT16` | **TODO** | The transaction type. |
| `LoanBrokerID` | :heavy_check_mark: | `string` | `HASH256` | `N/A` | The Loan Broker ID to deposit First-Loss Capital. |
| `Amount` | :heavy_check_mark: | `object` | `NUMBER` | 0 | The Fist-Loss Capital amount to deposit. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

sfAmount is an AMOUNT field, which requires currency, issuer, etc. I assume that's not what you want, in which case, we've got sfNumber

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ximinez I'm just getting to reviewing the comments now. Are you suggesting we rename Amount field to Number ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Tapanito Either

  • Change the type of Amount to AMOUNT to match the pre-existing defintion.
  • OR Change the name of Amount to an existing NUMBER field, or a new field that will be created as NUMBER. It could be Number, but it doesn't have to.

| ----------------- | :----------------: | :-------: | :-----------: | :-----------: | :------------------------------------------------------------ |
| `TransactionType` | :heavy_check_mark: | `string` | `UINT16` | **TODO** | Transaction type. |
| `LoanBrokerID` | :heavy_check_mark: | `string` | `HASH256` | `N/A` | The Loan Broker ID from which to withdraw First-Loss Capital. |
| `Amount` | :heavy_check_mark: | `object` | `NUMBER` | 0 | The amount of Vault asset to withdraw. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here: sfAmount is an AMOUNT field, which requires currency, issuer, etc. I assume that's not what you want, in which case, we've got sfNumber

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it's worth keeping it as amount, and chaking the type to amount too? Just in case, in the future, we want to support depositing different cover assets?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it's worth keeping it as amount, and chaking the type to amount too? Just in case, in the future, we want to support depositing different cover assets?

But doesn't the deposit go into the Single Asset Vault? That can't hold different types of assets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it's worth keeping it as amount, and chaking the type to amount too? Just in case, in the future, we want to support depositing different cover assets?

But doesn't the deposit go into the Single Asset Vault? That can't hold different types of assets.

But one possible reason to keep this as an Amount is to ensure that it's the right kind of amount, and that the broker doesn't get their asset types mixed up. In that case, it would fail if its the wrong kind of asset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ximinez , let's keep it as Amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ximinez , what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ximinez , what do you think?

Yeah, I'm fine with keeping it Amount, but the type MUST be changed to AMOUNT.

@Tapanito
Copy link
Contributor Author

Tapanito commented Mar 19, 2025 via email

@ximinez
Copy link
Collaborator

ximinez commented Mar 20, 2025

No, since we introduced a pseudo-account for the LoanBroker, the deposit will go there

Right, I wasn't thinking about that, but if there is a loan default, and funds have to be paid out of the Cover Amount, wouldn't that go to the SAV?

@Tapanito
Copy link
Contributor Author

No, since we introduced a pseudo-account for the LoanBroker, the deposit will go there

Right, I wasn't thinking about that, but if there is a loan default, and funds have to be paid out of the Cover Amount, wouldn't that go to the SAV?

Yes, 100%, the cover amount liquidation, in essence, will become a transfer of asset from one pseduo-account to another, with some accounting state changes.

@ximinez
Copy link
Collaborator

ximinez commented Mar 21, 2025

No, since we introduced a pseudo-account for the LoanBroker, the deposit will go there

Right, I wasn't thinking about that, but if there is a loan default, and funds have to be paid out of the Cover Amount, wouldn't that go to the SAV?

Yes, 100%, the cover amount liquidation, in essence, will become a transfer of asset from one pseduo-account to another, with some accounting state changes.

Right, so they should be the same currency, no? I'm fine with it being an STAmount as long as we're on the same page that the transaction should fail if it's not the same currency as the SAV.

| `Flags` | | `string` | `UINT32` | 0 | Specifies the flags for the Lending Protocol. |
| `Data` | | `string` | `BLOB` | None | Arbitrary metadata in hex format. The field is limited to 256 bytes. |
| `ManagementFeeRate` | | `number` | `UINT16` | 0 | The 1/10th basis point fee charged by the Lending Protocol Owner. Valid values are between 0 and 10000 inclusive. |
| `DebtMaximum` | | `number` | `NUMBER` | 0 | The maximum amount the protocol can owe the Vault. The default value of 0 means there is no limit to the debt. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| `DebtMaximum` | | `number` | `NUMBER` | 0 | The maximum amount the protocol can owe the Vault. The default value of 0 means there is no limit to the debt. |
| `DebtMaximum` | | `number` | `NUMBER` | 0 | The maximum amount the protocol can owe the Vault. The default value of 0 means there is no limit to the debt. Must not be negative. |

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should this value have a maximum? Maybe maxMPTokenAmount?

std::uint64_t constexpr maxMPTokenAmount = 0x7FFF'FFFF'FFFF'FFFFull;

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.

3 participants